-
Notifications
You must be signed in to change notification settings - Fork 1.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
Added routing exporter #1611
Added routing exporter #1611
Conversation
cc @tigrannajaryan, @jrcamp, as you were involved in the discussion for #1260. |
4115bf6
to
d45723c
Compare
Manually tested with the following config: receivers:
otlp/acme:
protocols:
grpc:
endpoint: localhost:55680
jaeger:
protocols:
thrift_compact:
processors:
exporters:
logging: {}
jaeger/default:
endpoint: localhost:24250
insecure: true
otlp/acme:
endpoint: localhost:55680
insecure: true
headers:
"x-tenant": "acme"
jaeger/acme:
endpoint: localhost:14250
insecure: true
routing:
from_attribute: X-Tenant
default_exporters: jaeger/default
table:
- value: acme
exporters: [jaeger/acme]
service:
pipelines:
traces:
receivers: [jaeger]
processors: []
exporters: [logging, otlp/acme]
traces/multitenant:
receivers: [otlp/acme]
processors: []
exporters: [logging, routing]
traces/routing:
exporters:
- jaeger/default
- jaeger/acme The
The
When the |
d45723c
to
c919159
Compare
As @tigrannajaryan and @jrcamp might be too busy, would you be able to review this one, @bogdandrutu, @dmitryax ? |
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.
The confusing part to me is the reference to the context. The end user likely knows nothing or very little about the context propagated inside the Collector. We need to make it much more clearer for the user to understand this.
I would also add this component to the contrib repo for now. If there is evidence that this is essential in the Collector we can then move it to the core repo.
As this depends on #1609, this PR is blocked waiting on a resolution for that. Once a decision there is made, I'll work on the review comments and on the pending items from this PR's description. |
Isn't there a discussion/plan on moving contrib to this main repo? |
No definitive plan yet. |
Closes open-telemetry#1260 Signed-off-by: Juraci Paixão Kröhling <[email protected]>
7dfc173
to
72cfed4
Compare
@tigrannajaryan, @nilebox, I rebased this PR and changed it to be a processor instead of exporter, as discussed in #1609. |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
For reference, here's how to perform a manual test for this feature: Configuration receivers:
otlp/acme:
protocols:
grpc:
endpoint: localhost:55680
jaeger:
protocols:
thrift_compact:
processors:
routing:
from_attribute: X-Tenant
default_exporters: jaeger/default
table:
- value: acme
exporters:
- jaeger/acme
batch:
exporters:
logging: {}
jaeger/default:
endpoint: localhost:24250
insecure: true
otlp/acme:
endpoint: localhost:55680
insecure: true
headers:
"x-tenant": "acme"
jaeger/acme:
endpoint: localhost:14250
insecure: true
service:
pipelines:
traces:
receivers:
- jaeger
processors: []
exporters:
- logging
- otlp/acme
traces/multitenant:
receivers: [otlp/acme]
processors:
- routing
exporters:
- logging
- jaeger/default
- jaeger/acme Then, start a local Jaeger server (referenced as "jaeger/acme" in the config): $ go run ./cmd/all-in-one --processor.jaeger-compact.server-host-port=16831 --query.static-files=./jaeger-ui/packages/jaeger-ui/build --log-level=debug And a second instance, from a container ("jaeger/default" in the config): $ podman run -p 24250:14250 -p 26686:16686 jaegertracing/all-in-one --log-level=debug Jaeger's $ go run ./cmd/tracegen -traces 10 Check "jaeger/acme" (http:https://localhost:16686), and you'll see the 10 traces there. Change the configuration, so that the line with "x-tenant" has "globex" as value and reload otelcol. Run In a production setup, each tenant would have its own otelcol instance, with the appropriate |
Codecov Report
@@ Coverage Diff @@
## master #1611 +/- ##
==========================================
+ Coverage 92.40% 92.44% +0.04%
==========================================
Files 265 267 +2
Lines 19981 20064 +83
==========================================
+ Hits 18464 18549 +85
+ Misses 1090 1089 -1
+ Partials 427 426 -1
Continue to review full report at Codecov.
|
The CI failed at
|
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
And the CI error is now gone O_O |
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 move this to contrib repo? With all the limitations around what other processors it can be used with I do not feel that it is ready for the core.
Closed in favor of open-telemetry/opentelemetry-collector-contrib#907 |
Closes open-telemetry/opentelemetry-collector#1260 Supersedes open-telemetry/opentelemetry-collector#1611 **Testing:** unit tests + manual tests (see open-telemetry/opentelemetry-collector#1611) **Documentation:** README included. Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Included all directives from the specification, clarify english, and translate specifics for the Go language.
…y#1611) Bumps [boto3](https://github.com/boto/boto3) from 1.23.7 to 1.23.8. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.23.7...1.23.8) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This PR depends on #1609, or a similar solution that will deliver all available exporters as part of
host.GetExporters
.While I believe this is still in draft mode, I would like to validate the direction this is going.
Open points to discuss:
processors
out for now, as it's probably more complex than exporters, given that there are nohost.GetProcessors
Signed-off-by: Juraci Paixão Kröhling [email protected]