Added routing exporter #1611
Added routing exporter #1611
Conversation
jpkrohling
requested review from
bogdandrutu,
dmitryax,
james-bebbington,
nilebox,
owais,
pjanotti and
tigrannajaryan
as code owners
|
cc @tigrannajaryan, @jrcamp, as you were involved in the discussion for #1260. |
jpkrohling
force-pushed
the
jpkrohling/issue1260
branch
2 times, most recently
from
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/acmeThe The When the |
jpkrohling
force-pushed
the
jpkrohling/issue1260
branch
from
d45723c to
c919159
Compare
|
As @tigrannajaryan and @jrcamp might be too busy, would you be able to review this one, @bogdandrutu, @dmitryax ? |
tigrannajaryan
left a comment
tigrannajaryan
left a comment
There was a problem hiding this comment.
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]>
jpkrohling
force-pushed
the
jpkrohling/issue1260
branch
from
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/acmeThen, 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=debugAnd 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=debugJaeger's $ go run ./cmd/tracegen -traces 10Check "jaeger/acme" (http://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 |
tigrannajaryan
left a comment
tigrannajaryan
left a comment
There was a problem hiding this comment.
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>
2 participants
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:
processorsout for now, as it's probably more complex than exporters, given that there are nohost.GetProcessorsSigned-off-by: Juraci Paixão Kröhling [email protected]