Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Jaeger remote sampler #1089

Merged
merged 8 commits into from
Apr 23, 2020
Merged

Add Jaeger remote sampler #1089

merged 8 commits into from
Apr 23, 2020

Conversation

pavolloffay
Copy link
Member

Resolves #796

This PR adds Jaeger remote sampler. The sampling configuration is retrieved via gRPC (as opposed to thrift in Jaeger clients).

Othe notable changes:

  • Added proto for Jaeger sampling model with service
  • The proto is generated to the same package as Jaeger gRPC exporter (io.opentelemetry.exporters.jaeger.proto.api_v2), this differs from the package defined in Jaeger proto (io.jaegertracing.api_v2)
  • Added RateLimitingSampler implementation from Jaeger
  • There is e2e test with Testcontainers and mock proto service test

Signed-off-by: Pavol Loffay [email protected]

pavolloffay
pavolloffay commented Apr 8, 2020
View reviewed changes
exporters/jaeger/src/main/java/io/opentelemetry/exporters/jaeger/JaegerRemoteSampler.java Outdated
private static final Logger logger = Logger.getLogger(JaegerRemoteSampler.class.getName());

private static final int DEFAULT_POLLING_INTERVAL_MS = 60000;
private static final Sampler INITIAL_SAMPLER = Samplers.probability(0.001);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial sampler is used before the configuration is obtained from the server.

I am not sure whether we should use this default from Jaeger clients or use default sampling probability from OTEL.

@pavolloffay
Copy link
Member Author

cc) @yurishkuro

Also as I mention on the email thread, this PR does not include double defaultLowerBoundTracesPerSecond and defaultUpperBoundTracesPerSecond

@yurishkuro
Copy link
Member

High-level question - why does this need to be JaegerRemoteSampler, instead of a general mechanism in OTel SDK with dynamically updatable configuration? Putting it under "jaeger" namespace, while also keeping it in the official SDK, sounds like technical debt to me.

@bogdandrutu
Copy link
Member

@yurishkuro great point. I think we should start considering defining a config protocol for OTLP. We have a starting point in the definition of the TraceConfig. We need to define a gRPC service/method.

@bogdandrutu
Copy link
Member

@yurishkuro my initial point was if a current user of Jaeger library wants to move to OpenTelemetry we need to support the current Jaeger way to configure sampling, was not thinking to add a new protocol.

@yurishkuro
Copy link
Member

yurishkuro commented Apr 8, 2020 •
edited
Loading

my initial point was if a current user of Jaeger library wants to move to OpenTelemetry we need to support the current Jaeger way to configure sampling.

It may sound like a good idea, but what it means in practice is that

  1. users change their apps from Jaeger/OT to OTel SDK, but keep running Jaeger collectors
  2. then they switch to OTel collectors (which may still expose Jaeger sampling formats)
  3. then they need to change their apps again (at least reconfigure) to let the SDK consume its native sampling formats

I think this is unnecessary complicated way, which also puts the burden on OTel SDK to support Jaeger sampling formats natively. I would rather:

  1. Users first switch from Jaeger collectors to OTel collectors (or jaeger-otel-collector, which will be included in the next release). It's a backend change that can be easily done in centralized manner.
  2. Then they start changing their apps to use OTel SDK, which simultaneously switches them to OTel-native sampling format.

The second upgrade path is simpler for the end user, and does not require technical debt in OTel SDK.

@pavolloffay
Copy link
Member Author

pavolloffay commented Apr 9, 2020 •
edited
Loading

So what do we do with this PR, shall I close it?

I can start the proposal/design in OTLP.

I do agree with @yurishkuro posts. We have discussed this feature on the OTEL java call. The consensus for implementing this was to have something ready for early adopters that are using Jaeger, and also showcase that a "vendor" specific functionality can be added to the OTEL SDK.

@pavolloffay
Copy link
Member Author

@bogdandrutu @carlosalberto @yurishkuro can you please comment on my last comment?

@yurishkuro
Copy link
Member

If we're adding this as "vendor specific" extension, can it be in contrib somewhere?

@pavolloffay
Copy link
Member Author

@yurishkuro it can be moved to a separate artifact in https://github.com/open-telemetry/opentelemetry-java/tree/master/contrib.

I just need to know if I should close this or continue.

@jkwatson
Copy link
Contributor

If we want this, I think moving it to a contrib module is the right way to go, until we have hashed out the details of the OTLP config option.

@pavolloffay pavolloffay force-pushed the jaeger-remote-sampler branch from 266cd76 to e119812 Compare April 20, 2020 14:46
@pavolloffay
Copy link
Member Author

I have moved the remote sampler to a separate sdk_contrib artifact and fixed review comments. Can somebody review?

@pavolloffay pavolloffay force-pushed the jaeger-remote-sampler branch from 909f16d to 0ad014b Compare April 21, 2020 08:11
@pavolloffay
Cosmetic changes
db628ee 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Add readme
755ce91 
Signed-off-by: Pavol Loffay <[email protected]>
jkwatson
jkwatson approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak to the correctness, as I'm not a Jaeger expert, but this looks fine from a code quality perspective.

@carlosalberto
Copy link
Contributor

Not a Jaeger expert neither, but this looks great to me. Maybe @yurishkuro can provide a final review? Else, we definitely should merge and start letting users play with this one ;)

@pavolloffay
Copy link
Member Author

Can somebody merge if it is good to go?

@yurishkuro yurishkuro merged commit 610c92f into open-telemetry:master Apr 23, 2020
davebarda pushed a commit to davebarda/opentelemetry-java that referenced this pull request Apr 24, 2020
@pavolloffay @davebarda
Add Jaeger remote sampler (open-telemetry#1089)
d4d5d58 
* Add Jaeger remote sampler

Signed-off-by: Pavol Loffay <[email protected]>

* Add tests

Signed-off-by: Pavol Loffay <[email protected]>

* Add remote sampler to all BOM

Signed-off-by: Pavol Loffay <[email protected]>

* Make docker test optional

Signed-off-by: Pavol Loffay <[email protected]>

* Use scheduled thread pool

* Use daemon thread factory

Signed-off-by: Pavol Loffay <[email protected]>

* Cosmetic changes

Signed-off-by: Pavol Loffay <[email protected]>

* Add readme

Signed-off-by: Pavol Loffay <[email protected]>
@jkwatson jkwatson added this to the May Release milestone May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@jkwatson jkwatson jkwatson approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@arminru arminru Awaiting requested review from arminru

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@songy23 songy23 Awaiting requested review from songy23

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
May Release
Development

Successfully merging this pull request may close these issues.

Implement remotely configurable sampling for Jaeger exporter
5 participants
@pavolloffay @yurishkuro @bogdandrutu @jkwatson @carlosalberto