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.

Sign up for GitHub

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

Jump to bottom

Fix textfile crashes with duplicate metrics #759

Merged
merged 2 commits into from Jun 24, 2021
Merged

Fix textfile crashes with duplicate metrics #759

merged 2 commits into from Jun 24, 2021

Conversation

breed808
Copy link
Contributor

Resolves #755

@breed808 breed808 requested a review from a team as a code owner April 18, 2021 04:25
@carlpett
Copy link
Collaborator

Just making sure I read this correctly: This PR will check for duplicates within each file (but not across multiple files), and drop the entire file in case there are duplicates? We would still expose those files that do not have duplicates, but set collector_success to 0?
There was some work on this a couple of years ago in #274 which might have some valuable historical context as well.

@breed808
Copy link
Contributor Author

Yes, that's correct. Looking at the link you provided I realise this implementation is somewhat naive, as it won't detect duplicates in different files.

@carlpett
Copy link
Collaborator

The strictest/"most correct" would probably be to merge all textfiles, and then check the complete set for validity. If it is not valid, we reject the entire textfile collector results, and emit no metrics at all.
Do you think that'd be reasonably easy to implement? I guess it'd involve a bunch of buffering, or trying to do something clever...

@breed808
Copy link
Contributor Author

breed808 commented May 1, 2021

It might be possible by stuffing each MetricFamily into a slice defined before the file loop, then checking for duplicates 🤔
I'll have a look and see if I can get it working.

@breed808
Fix textfile crashes with duplicate metrics
4293497 
Signed-off-by: Ben Reedy <breed808@breed808.com>
@breed808
Copy link
Contributor Author

I've got this working and passing CI, have a look and let me know if there's any concerns.

carlpett
carlpett reviewed May 17, 2021
View reviewed changes
Copy link
Collaborator

@carlpett carlpett left a comment

Choose a reason for hiding this comment

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

Nice! Some minor questions

collector/textfile.go Outdated Show resolved Hide resolved
collector/textfile.go Outdated Show resolved Hide resolved
collector/textfile.go Outdated Show resolved Hide resolved
collector/textfile.go Outdated Show resolved Hide resolved
@breed808
Check duplicates across entire textfile set
5072879 
Check all textfile metrics will be checked for duplicates. If duplicates
are detected, drop all metrics and log error.

Signed-off-by: Ben Reedy <breed808@breed808.com>
@breed808 breed808 requested a review from carlpett June 18, 2021 23:43
carlpett
carlpett approved these changes Jun 21, 2021
View reviewed changes
Copy link
Collaborator

@carlpett carlpett left a comment

Choose a reason for hiding this comment

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

Sorry, missed the push! Looks good, very nice work!

@breed808 breed808 merged commit 4b2cd0a into prometheus-community:master Jun 24, 2021
@breed808 breed808 deleted the textfile branch June 24, 2021 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlpett carlpett carlpett approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invalid textfile collector file brings down whole exporter
2 participants
@breed808 @carlpett