Skip to content

Commit ab721ae

Browse files
authored
[pkg/stanza] Simplify fingerprint updating (#31251)
Depends on #31298 Fixes #22936 This PR changes the way readers update their fingerprints. Currently, when `reader.ReadToEnd` is called, it creates a scanner and passes itself (the reader) in as the `io.Reader` so that a custom implementation of `Read` will be used by the scanner. Each time the scanner calls `Read`, we try to perform appropriate reasoning about whether the data we've read should be appended to the fingerprint. The problem is that the correct positioning of the bytes buffer is in some rare cases different than the file's "offset", as we track it. See example [here](#22937 (comment)). There appear to be two ways to solve this. A "simple" solution is to independently determine the file handle's current offset with a clever use of `Seek`, ([suggested here](https://stackoverflow.com/a/10901436/3511338). Although this does appear to work, it leaves open the possibility that the fingerprint is corrupted because _if the file was truncated_, we may be updating the fingerprint with incorrect information. The other solution, proposed in this PR, simply has the custom `Read` function set a flag to indicate that the fingerprint _should_ be updated. Then, just before returning from `ReadToEnd`, we create an entirely new fingerprint. This has the advantage of not having to manage any kind of append operations, but also allows the the opportunity to independently check that the fingerprint has not been altered by truncation. Benchmarks appear to show all three solutions are close in performance.
1 parent 4e4e452 commit ab721ae

File tree

8 files changed

+511
-106
lines changed

8 files changed

+511
-106
lines changed
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: bug_fix
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
7+
component: receiver/filelog
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Fix issue where file fingerprint could be corrupted while reading.
11+
12+
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
13+
issues: [22936]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext: |
19+
20+
# If your change doesn't affect end users or the exported elements of any package,
21+
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
22+
# Optional: The change log or logs in which this entry should be included.
23+
# e.g. '[user]' or '[user, api]'
24+
# Include 'user' if the change is relevant to end users.
25+
# Include 'api' if there is a change to a library API.
26+
# Default: '[user]'
27+
change_logs: []

pkg/stanza/fileconsumer/config.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/fingerprint"
2121
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/header"
2222
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/reader"
23+
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/scanner"
2324
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/matcher"
2425
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/operator"
2526
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/operator/helper"
@@ -151,18 +152,19 @@ func (c Config) buildManager(logger *zap.SugaredLogger, emit emit.Callback, spli
151152
}
152153

153154
readerFactory := reader.Factory{
154-
SugaredLogger: logger.With("component", "fileconsumer"),
155-
FromBeginning: startAtBeginning,
156-
FingerprintSize: int(c.FingerprintSize),
157-
MaxLogSize: int(c.MaxLogSize),
158-
Encoding: enc,
159-
SplitFunc: splitFunc,
160-
TrimFunc: trimFunc,
161-
FlushTimeout: c.FlushPeriod,
162-
EmitFunc: emit,
163-
Attributes: c.Resolver,
164-
HeaderConfig: hCfg,
165-
DeleteAtEOF: c.DeleteAfterRead,
155+
SugaredLogger: logger.With("component", "fileconsumer"),
156+
FromBeginning: startAtBeginning,
157+
FingerprintSize: int(c.FingerprintSize),
158+
InitialBufferSize: scanner.DefaultBufferSize,
159+
MaxLogSize: int(c.MaxLogSize),
160+
Encoding: enc,
161+
SplitFunc: splitFunc,
162+
TrimFunc: trimFunc,
163+
FlushTimeout: c.FlushPeriod,
164+
EmitFunc: emit,
165+
Attributes: c.Resolver,
166+
HeaderConfig: hCfg,
167+
DeleteAtEOF: c.DeleteAfterRead,
166168
}
167169
knownFiles := make([]*fileset.Fileset[*reader.Metadata], 3)
168170
for i := 0; i < len(knownFiles); i++ {

pkg/stanza/fileconsumer/internal/reader/factory.go

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,18 @@ const (
2828

2929
type Factory struct {
3030
*zap.SugaredLogger
31-
HeaderConfig *header.Config
32-
FromBeginning bool
33-
FingerprintSize int
34-
MaxLogSize int
35-
Encoding encoding.Encoding
36-
SplitFunc bufio.SplitFunc
37-
TrimFunc trim.Func
38-
FlushTimeout time.Duration
39-
EmitFunc emit.Callback
40-
Attributes attrs.Resolver
41-
DeleteAtEOF bool
31+
HeaderConfig *header.Config
32+
FromBeginning bool
33+
FingerprintSize int
34+
InitialBufferSize int
35+
MaxLogSize int
36+
Encoding encoding.Encoding
37+
SplitFunc bufio.SplitFunc
38+
TrimFunc trim.Func
39+
FlushTimeout time.Duration
40+
EmitFunc emit.Callback
41+
Attributes attrs.Resolver
42+
DeleteAtEOF bool
4243
}
4344

4445
func (f *Factory) NewFingerprint(file *os.File) (*fingerprint.Fingerprint, error) {
@@ -58,16 +59,21 @@ func (f *Factory) NewReader(file *os.File, fp *fingerprint.Fingerprint) (*Reader
5859
}
5960

6061
func (f *Factory) NewReaderFromMetadata(file *os.File, m *Metadata) (r *Reader, err error) {
62+
// Trim the fingerprint if user has reconfigured fingerprint_size
63+
if len(m.Fingerprint.FirstBytes) > f.FingerprintSize {
64+
m.Fingerprint.FirstBytes = m.Fingerprint.FirstBytes[:f.FingerprintSize]
65+
}
6166
r = &Reader{
62-
Metadata: m,
63-
logger: f.SugaredLogger.With("path", file.Name()),
64-
file: file,
65-
fileName: file.Name(),
66-
fingerprintSize: f.FingerprintSize,
67-
maxLogSize: f.MaxLogSize,
68-
decoder: decode.New(f.Encoding),
69-
lineSplitFunc: f.SplitFunc,
70-
deleteAtEOF: f.DeleteAtEOF,
67+
Metadata: m,
68+
logger: f.SugaredLogger.With("path", file.Name()),
69+
file: file,
70+
fileName: file.Name(),
71+
fingerprintSize: f.FingerprintSize,
72+
initialBufferSize: f.InitialBufferSize,
73+
maxLogSize: f.MaxLogSize,
74+
decoder: decode.New(f.Encoding),
75+
lineSplitFunc: f.SplitFunc,
76+
deleteAtEOF: f.DeleteAtEOF,
7177
}
7278

7379
if !f.FromBeginning {

pkg/stanza/fileconsumer/internal/reader/factory_test.go

Lines changed: 41 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/emittest"
1717
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/filetest"
1818
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/fingerprint"
19+
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/scanner"
1920
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/split"
2021
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/testutil"
2122
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/trim"
@@ -28,13 +29,14 @@ const (
2829

2930
func testFactory(t *testing.T, opts ...testFactoryOpt) (*Factory, *emittest.Sink) {
3031
cfg := &testFactoryCfg{
31-
fromBeginning: true,
32-
fingerprintSize: fingerprint.DefaultSize,
33-
maxLogSize: defaultMaxLogSize,
34-
encoding: unicode.UTF8,
35-
trimFunc: trim.Whitespace,
36-
flushPeriod: defaultFlushPeriod,
37-
sinkCallBufferSize: 100,
32+
fromBeginning: true,
33+
fingerprintSize: fingerprint.DefaultSize,
34+
initialBufferSize: scanner.DefaultBufferSize,
35+
maxLogSize: defaultMaxLogSize,
36+
encoding: unicode.UTF8,
37+
trimFunc: trim.Whitespace,
38+
flushPeriod: defaultFlushPeriod,
39+
sinkChanSize: 100,
3840
attributes: attrs.Resolver{
3941
IncludeFileName: true,
4042
},
@@ -46,33 +48,35 @@ func testFactory(t *testing.T, opts ...testFactoryOpt) (*Factory, *emittest.Sink
4648
splitFunc, err := cfg.splitCfg.Func(cfg.encoding, false, cfg.maxLogSize)
4749
require.NoError(t, err)
4850

49-
sink := emittest.NewSink(emittest.WithCallBuffer(cfg.sinkCallBufferSize))
51+
sink := emittest.NewSink(emittest.WithCallBuffer(cfg.sinkChanSize))
5052
return &Factory{
51-
SugaredLogger: testutil.Logger(t),
52-
FromBeginning: cfg.fromBeginning,
53-
FingerprintSize: cfg.fingerprintSize,
54-
MaxLogSize: cfg.maxLogSize,
55-
Encoding: cfg.encoding,
56-
SplitFunc: splitFunc,
57-
TrimFunc: cfg.trimFunc,
58-
FlushTimeout: cfg.flushPeriod,
59-
EmitFunc: sink.Callback,
60-
Attributes: cfg.attributes,
53+
SugaredLogger: testutil.Logger(t),
54+
FromBeginning: cfg.fromBeginning,
55+
FingerprintSize: cfg.fingerprintSize,
56+
InitialBufferSize: cfg.initialBufferSize,
57+
MaxLogSize: cfg.maxLogSize,
58+
Encoding: cfg.encoding,
59+
SplitFunc: splitFunc,
60+
TrimFunc: cfg.trimFunc,
61+
FlushTimeout: cfg.flushPeriod,
62+
EmitFunc: sink.Callback,
63+
Attributes: cfg.attributes,
6164
}, sink
6265
}
6366

6467
type testFactoryOpt func(*testFactoryCfg)
6568

6669
type testFactoryCfg struct {
67-
fromBeginning bool
68-
fingerprintSize int
69-
maxLogSize int
70-
encoding encoding.Encoding
71-
splitCfg split.Config
72-
trimFunc trim.Func
73-
flushPeriod time.Duration
74-
sinkCallBufferSize int
75-
attributes attrs.Resolver
70+
fromBeginning bool
71+
fingerprintSize int
72+
initialBufferSize int
73+
maxLogSize int
74+
encoding encoding.Encoding
75+
splitCfg split.Config
76+
trimFunc trim.Func
77+
flushPeriod time.Duration
78+
sinkChanSize int
79+
attributes attrs.Resolver
7680
}
7781

7882
func withFingerprintSize(size int) testFactoryOpt {
@@ -87,9 +91,15 @@ func withSplitConfig(cfg split.Config) testFactoryOpt {
8791
}
8892
}
8993

90-
func withMaxLogSize(maxLogSize int) testFactoryOpt {
94+
func withInitialBufferSize(size int) testFactoryOpt {
9195
return func(c *testFactoryCfg) {
92-
c.maxLogSize = maxLogSize
96+
c.initialBufferSize = size
97+
}
98+
}
99+
100+
func withMaxLogSize(size int) testFactoryOpt {
101+
return func(c *testFactoryCfg) {
102+
c.maxLogSize = size
93103
}
94104
}
95105

@@ -99,9 +109,9 @@ func withFlushPeriod(flushPeriod time.Duration) testFactoryOpt {
99109
}
100110
}
101111

102-
func withSinkBufferSize(n int) testFactoryOpt {
112+
func withSinkChanSize(n int) testFactoryOpt {
103113
return func(c *testFactoryCfg) {
104-
c.sinkCallBufferSize = n
114+
c.sinkChanSize = n
105115
}
106116
}
107117

0 commit comments

Comments
 (0)