-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: add consul agent service discovery #3834
Conversation
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
7444adc
to
109c18c
Compare
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
4737de8
to
f754f47
Compare
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
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.
I have only reviewed the wording of the documentation. I have not reviewed the code. With corrections, I approve the documentation of this feature.
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
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.
Looks Good!
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.
I skimmed it; looks pretty good to me. I'd probably prefer we use consul_agent
instead of consulagent
in our yaml configs & exposed meta labels, but that's largely a nitpick.
return | ||
} | ||
|
||
nodeName := self["Config"]["NodeName"].(string) |
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.
Part of me would like to see this access protected against panics, but I see the method signature doesn't support an error return argument. We could use logging + failure metrics here, but the panic on such an unexpected code path may be fine for now -- it's a quicker, cruder way to surface problems.
I think this is a change now or change never kind of thing @owen-d, I merged as is because there is an existing precedent with |
What this PR does / why we need it:
This PR adds a new service discovery option to promtail, which is an adaptation of the existing prometheus consul service discovery to use the consul agent APIs instead. This is beneficial for very large consul clusters, where the catalog api endpoints can get very big and slow. Since promtail is often deployed on every node in a cluster, it makes sense for it be able to leverage the local consul agent which is also often deployed on every node in a cluster.
Which issue(s) this PR fixes:
Fixes #3744
Special notes for your reviewer:
This is running in a test environment on GCP if you'd like to poke at it
Checklist