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

[chore][pkg/stanza] Remove unlimited memory for file paths #28491

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

djaglowski
Copy link
Member

@djaglowski djaglowski commented Oct 23, 2023

Follows #27823

This PR removes a list of all previously seen paths. The problem with this list is twofold. Primarily, it is unlimited in size. Although minor, this could grow indefinitely in some situations. Secondarily, it adds unnecessary complexity to the codebase.

The new approach simply logs a message whenever we open a file with contents which we've not seen in recent memory. Basically, it relies on the same mechanisms we use for tracking files for the purpose of whether or not to read them. While this is a slight change in user-facing functionality, I think it is reasonable considering the benefits.

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@atoulme
Copy link
Contributor

atoulme commented Oct 27, 2023

In my opinion this is worthy of a changelog. Our field folks really like those logs to make sure they have the right files reporting in, and they will be disoriented with this change. Just adding a bit to the changelog will help them make sense of this change.

@djaglowski djaglowski force-pushed the pkg-stanza-rm-seen-paths branch from b70a827 to 50fff22 Compare October 27, 2023 15:56
@djaglowski djaglowski merged commit 9685d4b into open-telemetry:main Oct 27, 2023
78 checks passed
@djaglowski djaglowski deleted the pkg-stanza-rm-seen-paths branch October 27, 2023 16:33
@github-actions github-actions bot added this to the next release milestone Oct 27, 2023
@andrzej-stencel
Copy link
Member

Can you explain why this is marked as a breaking change? Should users (which users) perform an action when upgrading?

@swiatekm
Copy link
Contributor

@astencel-sumo The receiver may now emit the same info log line about starting to watch a file multiple times for the same file in some circumstances. Though I agree we could do with a more succinct explanation of this change.

@djaglowski
Copy link
Member Author

I marked it as breaking based upon @atoulme's comment regarding field teams relying on specific behaviors. I'm open to changing it of course.

jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
@vasireddy99
Copy link
Contributor

vasireddy99 commented Nov 30, 2023

@astencel-sumo The receiver may now emit the same info log line about starting to watch a file multiple times for the same file in some circumstances. Though I agree we could do with a more succinct explanation of this change.

thanks for the explanation, can you please explain with an example circumstance where the receiver may emit same info is emitted. I think you meant this line right.

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.

7 participants