Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[receiver/filelog] Fix bug in delete_after_read #31384

Merged

Conversation

djaglowski
Copy link
Member

Fixes #31383

This enhances TestDeleteAfterRead in a way that replicates the problem, and fixes the issue by retaining metadata on a reader until the exported Close function is called. Previously, we were removing the metadata when delete called Close, but this precluded the opportunity for the caller of ReadToEnd to properly manage file metadata after deletion.

@djaglowski djaglowski marked this pull request as ready for review February 22, 2024 15:33
@djaglowski djaglowski requested review from a team and bryan-aguilar February 22, 2024 15:33
@prigio
Copy link

prigio commented Feb 22, 2024

Thank you @djaglowski, #31384 does not panic anymore, however I feel something is still off

When testing it on my local deployment, it raises an error for failing in seeking on the file which has been closed, and its Reader.file reference is still intact.

This causes a lot of spurious logs for a behavior which is actually as per configuration. On a sidenote, the Warn log "finding files: no files match the configured criteria" also causes a lot of spam.

The logs:

...
2024-02-22T16:43:05.316+0100	info	fileconsumer/file.go:261	Started watching file	{"kind": "receiver", "name": "filelog", "data_type": "logs", "component": "fileconsumer", "path": "data/file.ndjson"}
.. some logs the debug exporter ...
2024-02-22T16:43:05.516+0100	error	reader/reader.go:50	Failed to seek	{"kind": "receiver", "name": "filelog", "data_type": "logs", "component": "fileconsumer", "path": "data/file.ndjson", "error": "invalid argument"}
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/internal/reader.(*Reader).ReadToEnd
	github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza@v0.94.0/fileconsumer/internal/reader/reader.go:50
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer.(*Manager).preConsume.func1
	github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza@v0.94.0/fileconsumer/file_other.go:47
2024-02-22T16:43:11.716+0100	warn	fileconsumer/file.go:132	finding files: no files match the configured criteria	{"kind": "receiver", "name": "filelog", "data_type": "logs", "component": "fileconsumer"}

@djaglowski
Copy link
Member Author

Thanks @prigio. I've added another bit of handling so that we don't attempt to read "lost" files when we know they should have already been deleted. Looking into the problem, I think we may have an issue with detecting false positives when looking for "lost" files. However, I think this fix is enough to resolve your issue. I'll continue looking into the other issue separately.

@prigio
Copy link

prigio commented Feb 23, 2024

Thank you. I tried out your latest changes and it looks good now.

@djaglowski djaglowski merged commit 166a7b4 into open-telemetry:main Feb 26, 2024
142 checks passed
@djaglowski djaglowski deleted the pkg-stanza-fileconsumer-darfix branch February 26, 2024 22:54
@github-actions github-actions bot added this to the next release milestone Feb 26, 2024
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
Fixes open-telemetry#31383

This enhances `TestDeleteAfterRead` in a way that replicates the
problem, and fixes the issue by retaining metadata on a reader until the
exported `Close` function is called. Previously, we were removing the
metadata when `delete` called `Close`, but this precluded the
opportunity for the caller of `ReadToEnd` to properly manage file
metadata after deletion.
LokeshOpsramp pushed a commit to opsramp/opentelemetry-collector-contrib that referenced this pull request May 5, 2024
Fixes open-telemetry#31383

This enhances `TestDeleteAfterRead` in a way that replicates the
problem, and fixes the issue by retaining metadata on a reader until the
exported `Close` function is called. Previously, we were removing the
metadata when `delete` called `Close`, but this precluded the
opportunity for the caller of `ReadToEnd` to properly manage file
metadata after deletion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filelog receiver panics when delete_after_read is true
4 participants