-
Notifications
You must be signed in to change notification settings - Fork 486
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
loki.source.docker: deduplicate matching container IDs #6055
loki.source.docker: deduplicate matching container IDs #6055
Conversation
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
6624847
to
7348a5c
Compare
Signed-off-by: Paschalis Tsilias <[email protected]>
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.
Ultimately I think this makes sense, but I think we need to document it, since it might be surprising if you effectively input three targets (with the same ID) into loki.source.docker but the debug info only shows one.
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
I've added a paragraph to document this new behavior, hopefully it clears things up. |
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 but the docs change should get a review by @clayton-cornell
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.
Some minor changes to the doc input
Co-authored-by: Clayton Cornell <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]> Co-authored-by: Clayton Cornell <[email protected]>
PR Description
The
discovery.docker
component was initially built with Prometheus service discovery in mind; this means that it will report a different target for each port, network and network interface that a container exposes/belongs to, so that it can be scraped accordingly.This means, that when
discovery.docker
is used as an input to loki.source.docker, it will set up unnecessary load on the Docker API by trying to fetch logs for the same container ID multiple times. This PR makes it so that during a component Update, the targets are deduplicated based on the container ID, keeping only the first one.Which issue(s) this PR fixes
Related to: #4403
I'd like to gather some more evidence until we say this fixes #4403 outright, so keeping this PR in draft in the meantime.
Notes to the Reviewer
I've run a simplistic benchmark with a chatty container that exposes two ports, and as expected, the CPU time spent on reading logs is ~half here.
PR Checklist