-
Notifications
You must be signed in to change notification settings - Fork 509
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
Add configurable remote_write headers to metrics-generator #3175
Conversation
RemoteWriteConfigs: generateTenantRemoteWriteConfigs(s.cfg.RemoteWrite, s.tenantID, newHeaders, s.cfg.RemoteWriteAddOrgIDHeader, s.logger), | ||
}) | ||
if err != nil { | ||
level.Error(s.logger).Log("msg", "Failed to update remote write headers. Remote write will continue with old headers", "err", err) |
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.
do we know this is true? if it the prom remote storage fails to apply new configs do we know it continues with the old?
updateProcessors at the level above increments a counter metric on failure which i like. metrics_generator_active_processors_update_failed_total
. i would love to just have all of this reloaded and counted under the same metric, but that name is pretty opinionated.
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.
do we know this is true? if it the prom remote storage fails to apply new configs do we know it continues with the old?
Yes. If the update fails, the function returns and the original queue is left untouched https://github.com/prometheus/prometheus/blob/main/storage/remote/write.go#L148-L177.
updateProcessors at the level above increments a counter metric on failure which i like. metrics_generator_active_processors_update_failed_total. i would love to just have all of this reloaded and counted under the same metric, but that name is pretty opinionated.
Good point. Added!
bd8946f
to
36ff74e
Compare
@mapno Will we need doc updates for this? |
36ff74e
to
ff85f17
Compare
@mapno It looks like we'll need docs for authentication and configuration for this feature. I'll add a note to the issue as well. |
94cfe7c
to
55050bf
Compare
Dear, thanks for this feature, could it be that the remote_write_headers parameter is not added in the configuration page? https://grafana.com/docs/tempo/latest/configuration/#standard-overrides |
Dear I'm not sure if I'm doing something wrong, but it does not seem to work for me.
|
This also does not work: |
What this PR does:
Adds per-tenant configurable headers for remote_write requests in the metrics-generator.
It also adds envvar expansion to the overrides configuration.
Which issue(s) this PR fixes:
Fixes #2975
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]