-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Promtail] resolve issue with promtail not scraping target if only path changed in a simpler way that dont need mutex to sync threads #4599
Conversation
…in a simpler way that dont need mutex to sync threads Signed-off-by: Roger Steneteg <[email protected]>
level.Debug(s.log).Log("msg", "ignoring target, already exists", "labels", labels.String()) | ||
s.metrics.failedTargets.WithLabelValues("exists").Inc() | ||
} | ||
if _, ok := s.targets[key]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an promtail expert. Leave it @slim-bean for the final approval.
I still do think we need some tests to lock this behaviour!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that be great.
@slim-bean should we merge ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a chance to look at this. Looks great, thank you!
@rsteneteg may I ask if this PR is going to fix #3698 also? I fear it does not. |
@Mario-Hofstaetter it looks like this change would also fix #3698, since the path variable is used within the key it would no longer be seen as a duplicate and ignored. |
resolve issue with promtail not scraping target if only path changed in a simpler way that dont need mutex to sync threads
Signed-off-by: Roger Steneteg [email protected]
What this PR does / why we need it:
Resolves an issue in a better way, that don't require sharing variables between theads that would require a mutex to synchronize goroutines.
Which issue(s) this PR fixes:
Fixes #4581 #4460
Special notes for your reviewer:
Checklist