-
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
New component: Routing connector #19738
Comments
I guess, It looks good to me! @jpkrohling can you take a look at the configuration? I am happy to work on it, but I probably will be able to start on it sometime in the second half of April. |
@kovrus, good catch, just missed that one. I can start work on this today if you and/or @jpkrohling are willing to sponsor/co-own the component. |
I am happy to sponsor it! |
This looks good, but I would keep routing based on attributes out of the initial version, only based on resources and, eventually, context values (like auth or HTTP header). |
I have unfortunately not made much progress on this. I began an implementation but my proposed redesign of the configuration seems to have introduced many complications I did not anticipate. I do not believe this is a problem related to converting this component to a connector - just a problem with trying to redesign the configuration and evaluation of routing rules. Perhaps it is best to change the config as little as possible, though I do believe we should drop support for non-ottl statements. I don't expect to have additional time for this until May, so @kovrus, if you are still able to work on this please feel free to pick this up. Otherwise, I will circle back to this when possible. |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
The PR for the initial version of the routing connector is in review. It supports routing based on resource attributes via OTTL statements. I know that we would like to add context based routing as a follow up PR, but I wanted to gather some information on this issue on how we'd like to approach it. Do we foresee adding additional configuration for context based routing? If so, how would we like that to look? Is the plan for us to extend OTTL to be able to match contexts used by the connector, or something else? I just want to make sure we have some idea of how we'd like this to work so that we don't paint ourselves into a corner with the initial implementation. |
My understanding is that we should support context-based routing, but I'm not an expert on the subject. @jpkrohling and @kovrus, as code owners of the routing processor, can you weigh in on this?
Can you elaborate on this question? |
For context, I imagine something like: connectors:
routing:
spans:
- statement: metadata["X-Tenant-ID"] == "acme"
pipelines: [traces/1] Perhaps We use something like that in the header setter extension: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/headerssetterextension#configuration opentelemetry-collector-contrib/extension/headerssetterextension/internal/source/context.go Lines 19 to 32 in 7a1061d
The routing processor looks up only gRPC metadata in the context, which predates the client.Info facility. |
**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. * The routing connector needs to be a consumer in multiple pipelines (the routing processor can route to a single exporter). * Will be resolved by: open-telemetry/opentelemetry-collector#7840 * We need the ability to create connector routers to facilitate testing. I had to temporarily copy the fanoutconsumer package into the routing connector codebase due to package visibility issues. * Will be resolved by: open-telemetry/opentelemetry-collector#7673 We 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 --------- Co-authored-by: Daniel Jaglowski <[email protected]> Co-authored-by: Ruslan Kovalov <[email protected]>
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
Would be interested to broach the topic of context-aware routing again. My experience so far is that the routing connector works well, but necessarily destroys context. Having a context-aware routing connector would be much more efficient, as we don't need to unpack any of the payload in order to route it, and more easily support multi-tenant setups. As for how to approach it - how about running the connector in either I think this would be preferable, as I can't quite tell how the connector would work in a "joint mode" where we have routes that match both metadata and resource attributes: connectors:
routing:
default_pipelines: [traces/jaeger]
error_mode: ignore
match_once: false
table:
- statement: route() where metadata["X-Tenant"] == "acme"
pipelines: [traces/jaeger-acme]
- statement: where attributes["job"] == "firewall"
pipelines: [traces/jaeger-firewall] I see |
@verejoel, your proposal makes sense to me, and sounds fairly straightforward. Will we need a new config parameter to indicate the context, or where you envisioning that we could infer & enforce one or the other from the statements? |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping |
I've opened a draft PR with a proof of concept for routing by request context. I'm looking for early feedback on the design.
|
With #36067 merged, I think this issue can be closed once similar functionality is implemented for traces and metrics. I plan to work on this soon. |
@djaglowski Thanks for this - I will be very happy to test it with the next release :) awesome work! |
The purpose and use-cases of the new component
We should develop a connector version of the
routingprocessor
(and deprecate the processor version).The connector implementation would differ only in a couple minor ways:
table
key with context-specific keys.Example configuration for the component
Telemetry data types supported
traces, metrics, logs
Is this a vendor-specific component?
Sponsor (optional)
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: