-
Notifications
You must be signed in to change notification settings - Fork 2.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
[routingconnector] convert routing processor to a connector #21498
Conversation
9fd0104
to
bd42812
Compare
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.
@mwear, thanks for taking this on. I'm excited to see so much progress.
@djaglowski thanks for the initial round of feedback. I was wondering if you had any opinions on the two issues I ran into with the fanoutconsumer (mentioned in the description). I could work around the package visibility issue by introducing mock fanout consumers for the tests, but the actual fanout consumer is subtly complex, which introduces some risk. I have worked around the second issue, needing a connector be a receiver in multiple pipelines in order to get a fanout consumer, by adding that restriction to the routing connector; the processor did not have this restriction. Is that the right thing to do, or can we add the cabability to request a fanout consumer for a connector even if it is only a receiver in a single pipeline, the advantage being, that we can still request a consumer by name. Let me know if you have opinions on those issues. I was waiting to get some resolution on them before taking this out of draft. |
@mwear, regarding problem 1 noted above: I see what you are saying. I think what it boils down to is that we need a way for tests to create a If we move type tracesRouterTestOption struct {
id component.ID
cons consumer.Traces
}
func WithNop(id component.ID) tracesRouterTestOption {
return tracesRouterTestOption{ id: id, cons: consumertest.NewNop() }
}
func WithTracesSink(id component.ID) tracesRouterTestOption {
return tracesRouterTestOption{ id: id, cons: &consumertest.TracesSink{} }
}
func NewTracesRouterSink(opts ...tracesRouterTestOption) connector.TracesRouter {
consumers := make(map[component.ID]consumer.Traces, numRoutes)
for _, opt := range opts {
consumers[opt.id] = opt.cons
}
return fanoutconsumer.NewTracesRouter(consumers)
} |
Regarding problem 2: I tend to think that a router really should expect a minimum of two downstream pipelines. It seems that anything that could be done with a router that emits to only one pipeline, could (and should) be done in a simpler way. |
@djaglowski: Thanks for the suggestions. I like your proposal for problem 1 and will work on a PR for it. I'm fine with leaving problem 2 as is. It makes sense to require multiple downstream pipelines, even though it introduces a slight difference in behavior for the connector vs the processor. |
I'll leave this in draft until there is some resolution on: open-telemetry/opentelemetry-collector#7673. I'll also be OOO next week (back on May 22nd). I welcome any feedback in the meantime, and will address it when I return. |
|
||
## Differences between the Routing Connector and Routing Processor | ||
|
||
- The connector will only route using [OTTL] statements which can only be applied to resource attributes. It does not support matching on context values at this time. |
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.
It seems that quite some users (including me) do routing based on the context values. Maybe after this PR we can follow up with the OTTL function that extracts values from context if it is possible.
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.
We can add context based routing as a follow up, but we should discuss how we'd like that to work so that we don't paint ourselves into a corner with the first pass.
I'm way behind my github notifications, but I'm placing this to the top of my queue. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR is waiting for @bogdandrutu's response here: #21498 (comment) |
@mwear, I think we should move forward with the temporarily copied fanout consumer code. WDYT? |
That works for me. Removing the copied code will be easy once the other changes land on core. Should I dust this off and bring it out of draft? |
@mwear, I'm all for it. 👍 |
22f51e5
to
2ece800
Compare
Co-authored-by: Ruslan Kovalov <ruslan.kovalov@grafana.com>
Now that open-telemetry/opentelemetry-collector#7840 merged, we can expect a fanout consumer to be passed consistently to a connector factory. We still check for an error casting the consumer to a connector router, but this would only happen if a breaking change was introduced to the connector internals.
because CI told me to: > dependabot.yml is out of date, please run "make gendependabot" and commit the changes in this PR
b81ce3f
to
f8ca0fa
Compare
A few comments have not been marked as resolved, but I believe all concerns have been addressed. @jpkrohling, @kovrus, please double check the open comments and raise issues if you would like to see additional changes. @mwear, thank you for all the work on this! This is a critical connector and I'm looking forward to making it available to users. |
Description:
This PR introduces a connector version of the routing processor. It currently supports routing based on resource attributes OTTL statements only. Context based routing will be added later in a followup PR.
There are two issues regarding fanout consumers that are being addressed in the core repo.
connector.*Router
s for tests opentelemetry-collector#7673We can address these issues as the relevant PRs land on the core repo.
cc: @djaglowski, @jpkrohling, @kovrus
Link to tracking Issue:
#19738
Testing:
Unit / manual
Documentation:
The readme has been ported from the routing processor