-
Notifications
You must be signed in to change notification settings - Fork 487
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
Support mirror traffic pipelines #474
Conversation
* introduce `remote_write` blocks and `batch` in Tempo config * deprecate `push_config` in Tempo config * support multiple backends for a single tracing pipeline
Wasn't really sure how to proceed with all the changes needed in |
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 think this is pretty reasonable but I'm going to tag @joe-elliott for a review.
Does batch need to be applied across all the exporters or is it possible to have (or want) per-endpoint batch settings?
* move Batch field down for clarity regarding its deprecation * remove unnecessary pointer receiver in exporter function * remove redundant check for {remote_write/push_config}.endpoint
b68aa01
to
fca5b8b
Compare
203101e
to
266d2ad
Compare
Changes config examples to use remote_write/batch instead of push_config
* Sort config pipelines' exporters to make them comparable Maps in Go are not sorted, so the exporters can end up in any order, making the pipeline's exporter slice sorted in that order. If the order is different for actualConfig and expectedConfig, the assertion will fail while both being essentially the same config.
Applied the suggested changes and updated the config examples, as well as the config in |
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.
One final thought about backwards compat.
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.
Thanks for changing the Tanka stuff too, great work :)
I think my open question from last time got missed. What are the odds someone is going to watch different batch settings per remote_write? Prometheus does this with its queue_config settings. If the same thing doesn't make as much sense for traces, then I think this PR is fine as-is. Otherwise, I think we might want to consider setting up one pipeline per exporter, maybe in a follow-up PR.
(I'm not sure if that actually works though, i.e., if OTel lets you re-use receivers across multiple pipelines.)
return exporters, nil | ||
} | ||
|
||
func (c *InstanceConfig) otelConfig() (*configmodels.Config, error) { |
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.
Can we log a warning somewhere here if push_config is configured to note that it's deprecated in favor of remote_write?
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.
Added in 53d7f96
Yes, sorry I totally missed it before. I'm not sure how often people are going to want per-exporter batch configs, but is something doable. The OTel collector lets you set up pipelines re-using components (i.e. receivers, processors, exporters) as much as you'd like. I think that different batch configs is most interesting when the exporters are different, which for the agent is not the case (we're always using the otlp exporter). But maybe @joe-elliott can give a better answer since he has more experience operating it. |
ca6cfcf
to
53d7f96
Compare
The PR as is covers the necessary cases. The OTEL collector does not allow a batch processor per exporter in a mirrored setup. There can be only one batch exporter for the pipeline that is then mirrored to the two exporters. Independent pipelines can (and should) have independent batch processors but this is already covered at the |
Thanks for the input! Merging. Thanks again @mapno! |
* Support mirroring a trace pipeline to multiple backends * introduce `remote_write` blocks and `batch` in Tempo config * deprecate `push_config` in Tempo config * support multiple backends for a single tracing pipeline * Update CHANGELOG * Apply some review suggestions * move Batch field down for clarity regarding its deprecation * remove unnecessary pointer receiver in exporter function * remove redundant check for {remote_write/push_config}.endpoint * Remove Batch from RemoteWrite config * Update config examples Changes config examples to use remote_write/batch instead of push_config * Fix flaky config test * Sort config pipelines' exporters to make them comparable Maps in Go are not sorted, so the exporters can end up in any order, making the pipeline's exporter slice sorted in that order. If the order is different for actualConfig and expectedConfig, the assertion will fail while both being essentially the same config. * Update /production config to use RemoteWrite * Undeprecate Batch from PushConfig * Add warning log when using push_config
PR Description
Adds support for multiple backends in a tracing pipeline.
push_config
is deprecated in favor ofremote_write
andbatch
, which configure the multiple exporters.remote_write
still usesPushConfig
, but the idea is to rename it and remove theBatch
field once thepush_config
is removed from the config.Which issue(s) this PR fixes
Closes #466
Notes to the Reviewer
PR Checklist