Add jaeger remote sampler to SDK configuration #1791

pavolloffay · 2021-07-05T09:40:23Z

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

Related to open-telemetry/opentelemetry-java#3368

This PR adds SDK configuration for jaeger_remote_sampler. The goal is to make the jaeger remote sampler available and configurable in auto-instrumentations.

cc) @tedsuo @anuraaga

pavolloffay · 2021-07-05T09:42:36Z

specification/sdk-environment-variables.md


 Depending on the value of `OTEL_TRACES_SAMPLER`, `OTEL_TRACES_SAMPLER_ARG` may be set as follows:

 - For `traceidratio` and `parentbased_traceidratio` samplers: Sampling probability, a number in the [0..1] range, e.g. "0.25". Default is 1.0 if unset.
+- For `jaeger_remote_sampler`: The value is a comma separated list of: pooling interval `poolingIntervalMs` in milliseconds and initial sampling rate `initialSamplingRate` in the [0..1] range. For example: `poolingIntervalMs=100,initialSamplingRate=0.25`.


apart from this the remote sampler needs 2 configuration options:

service name - it will (re)use otel.service.name

collector endpoint - it will (re)use otel.exporter.jaeger.endpoint

specification/sdk-environment-variables.md

anuraaga · 2021-07-05T09:47:34Z

specification/sdk-environment-variables.md


 Depending on the value of `OTEL_TRACES_SAMPLER`, `OTEL_TRACES_SAMPLER_ARG` may be set as follows:

 - For `traceidratio` and `parentbased_traceidratio` samplers: Sampling probability, a number in the [0..1] range, e.g. "0.25". Default is 1.0 if unset.
+- For `jaeger_remote_sampler`: The value is a comma separated list of: pooling interval `poolingIntervalMs` in milliseconds and initial sampling rate `initialSamplingRate` in the [0..1] range. For example: `poolingIntervalMs=100,initialSamplingRate=0.25`.


I would use initial_sampler and initial_sampler_arg instead of (or possibly in addition to) just a rate. The intent is to use a different Otel Sampler as initial, not to restrict it I think. This is reflected in how the code API accepts Sampler as the argument

the API in OTEL might support this but let me see if it is supported in Jaeger. The main use-case is to provide a migration path for Jaeger users.

In the long term there will be a native OTEL remote sampling implementation.

In Jaeger Java SDK the default sampler type in environment config is always percentage-based https://github.com/jaegertracing/jaeger-client-java/blob/568ab68ec2ac5e9e811b1d0d76f752f3242d45d3/jaeger-core/src/main/java/io/jaegertracing/Configuration.java#L394

The programmatic API allows to supply any sampler like in OTEL, but not via env variables.

@yurishkuro please loop in here. Do we need to set the initial sampler to rate limiting type? Was this something users were using? We didn't define environment configuration for it.

yurishkuro · 2021-07-05T15:35:39Z

specification/sdk-environment-variables.md


 Depending on the value of `OTEL_TRACES_SAMPLER`, `OTEL_TRACES_SAMPLER_ARG` may be set as follows:

 - For `traceidratio` and `parentbased_traceidratio` samplers: Sampling probability, a number in the [0..1] range, e.g. "0.25". Default is 1.0 if unset.
+- For `jaeger_remote`: The value is a comma separated list of: pooling interval `poolingIntervalMs` in milliseconds and initial sampling rate `initialSamplingRate` in the [0..1] range. For example: `poolingIntervalMs=100,initialSamplingRate=0.25`.


it will (re)use otel.exporter.jaeger.endpoint

Why not have a separate setting like server=? Reusing otel.exporter.jaeger.endpoint is both confusing (what does exporter have to do with sampler?) and inflexible, as it would couple exporter and sampler, even though someone might want to have them at different addresses.

Also, is this a gRPC or HTTP address?

poolingIntervalMs=100

It's polling, not pooling.

I would use a different, more realistic example value than 100ms, like 10000. Does this even need to be in ms (is that the convention for other vars?), or can it be in seconds?

`initialSamplingRate=0.25

I would rename initialSamplingRate to defaultSamplingRate.

And I would add clear documentation of the sub-properties:

For jaeger_remote: The value is a comma separated list of:

pollingIntervalMs in milliseconds indicating how often the sampler will poll the backend for updates to sampling strategy

defaultSamplingRate in the [0..1] range, which is used as the sampling probability when the backend cannot be reached to retrieve a sampling strategy. This value stops having an effect once a sampling strategy is retrieved successfully, as the remote strategy will be used until a new update is retrieved.

server (name tbd) - the address of gRPC (or http?) server that will serve the sampling strategy for the service (link to schema)

Example: pollingIntervalMs=10000,defaultSamplingRate=0.25,server=...

I was thinking about adding the endpoint/address to the arguments. I think it makes sense. 99.99999% users will use the same address as for the exporter, but having it in the argument makes it easy understandable what needs to be configured.

I don't like server. I will use endpoint to be consistent with other names in the SDK. The address could be perhaps both HTTP and grpc (like in case of OTLP), this offers flexibility for the SDKs to implement or or the other. I will check if the OTELcol exposes both protocols.

pavolloffay · 2021-07-06T10:00:37Z

PR updated

specification/sdk-environment-variables.md

yurishkuro · 2021-07-06T13:44:21Z

specification/sdk-environment-variables.md

-
-## Batch Span Processor
+- For `jaeger_remote`: The value is a comma separated list:
+  - `endpoint`: the endpoint of gRPC server that serves the sampling strategy for the service ([sampling.proto](https://github.com/jaegertracing/jaeger-idl/blob/master/proto/api_v2/sampling.proto)).


What is the syntax, just host:port? Is TLS supported?

Below is an example of the endpoint. I will add it here to be more obvious...

TLS could be supported by providing a scheme in the endpoint like it is done for OTLP e.g. https://github.com/open-telemetry/opentelemetry-java/blob/43f1ecf7ede49104c2aac1c9e5448b7cf7859ce7/exporters/otlp/trace/src/main/java/io/opentelemetry/exporter/otlp/trace/OtlpGrpcSpanExporterBuilder.java#L131. We could add another option with path to the cert.

anuraaga · 2021-07-06T15:15:29Z

specification/sdk-environment-variables.md

@@ -56,12 +56,16 @@ Known values for `OTEL_TRACES_SAMPLER` are:
 - `"parentbased_always_on"`: `ParentBased(root=AlwaysOnSampler)`
 - `"parentbased_always_off"`: `ParentBased(root=AlwaysOffSampler)`
 - `"parentbased_traceidratio"`: `ParentBased(root=TraceIdRatioBased)`
+- `"jaeger_remote"`: `JaegerRemoteSampler`


I want to give one more push for just jaeger. Doesn't the syntax make most cognitive sense looking something like OTEL_EXPORTER=jaeger OTEL_PROPAGATORS=jaeger OTEL_SAMPLER=jaeger?

Not for me, I would still advocate for jaeger_remote. Just jaeger seems ambiguous to me.

I would suggest that rather than calling out a named "jaeger_remote" this should be perhaps "custom" or something similar. The resulting value can still be "JeagerRemoteSampler"

And then the configuration below would be specific to the instance of the custom sampler and would be decoded by the sampler.

So the OTEL_TRACES_SAMPLER_ARG field would then be <CustomSampler>;<CustomSamplerSpecificArgs>

So an example: JaegerRemoteSampler;endpoint=localhost:14250,pollingIntervalMs=5000,initialSamplingRate=0.25

@MSNev this complicates the parsing and still requires the spec to call out valid custom samplers & schemes. I don't see a lot of benefits over the current proposed format.

It means we don't have to spec every sampler and the sdk developers don't have to make any code changes to support new samplers (beyond creating the sampler) so in the spec we just say use the custom sampler config.

still requires the spec to call out valid custom samplers & schemes

Only if we want them to be officially provided, ie. we don't have to call out "valid custom samplers" we just say you can add a customer sampler using this format with the JaegerRemote Sampler in this case providing an example. This could include a simple section on available/provided customer samplers as I can envisage that all environments won't necessarily want or need all samplers

So another example could just be

OTEL_TRACES_SAMPLER=custom OTEL_TRACES_SAMPLER_ARG=MyCustomSampler;<MyCustomSamplerSpecificArgs>

So the value after the ';' is simple "passed" to the custom sampler itself to process rather than enforcing every customer sampler to conform to something it doesn't need. eg. There could be a sampler that doesn't take any args.

Available custom samplers

Jaeger Remote

Name: JaegerRemoteSampler
Args: ... as defined ...

Example Usage:

OTEL_TRACES_SAMPLER=custom OTEL_TRACES_SAMPLER_ARG=JaegerRemoteSampler;endpoint=localhost:14250,pollingIntervalMs=5000,initialSamplingRate=0.25

still requires the spec to call out valid custom samplers & schemes
Only if we want them to be officially provided, ie. we don't have to call out "valid custom samplers"

Well, the whole point of this PR is to have Jaeger's remote sampler officially supported, so how does it benefit from being classified as "custom" yet still being fully spec-ed?

And even for unofficial custom samplers, I still don't see the value in the indirection. If someone wants to use a truly custom sampler that is not in the spec, they can still read its name from OTEL_TRACES_SAMPLER, and the SDK needs to have a configurable factory that could recognize the name and invoke the appropriate builder.

btw, #1829 could be a good candidate for OTEL_TRACES_SAMPLER=custom

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

pavolloffay · 2021-07-08T16:46:28Z

@tedsuo could you please review as well?

Here is OTEPS for OTEL remote sampler open-telemetry/oteps#167. I would like to get this PR merged to have a clear migration path for Jaeger users. The native OTEL based approach will take a longer time to get it to fully working solution.

specification/sdk-environment-variables.md

tedsuo · 2021-07-22T15:50:47Z

Before merging this work, I have a couple of requests:

First, can we get a specification for the current Jaeger remote sampling format and protocol OpenTelemetry is expected to support via a sampling plugin? Either written here as a spec, or linked to in the Jaeger documentation?

Second, is it possible to get a commitment from the Jaeger team that future work on remote sampling will be moved over to the OpenTelemetry Sampling SIG? In other words, we'd like to add these sampling mechanisms in order to provide legacy support for existing Jaeger backends, which will allow the Jaeger community to switch over to using the OTel clients. But there are some concerns about having two independent groups working on sampling mechanisms in an uncoordinated fashion. @pavolloffay @yurishkuro @jpkrohling for future sampling work, is the Jaeger community comfortable with specifying those mechanisms as part of an OpenTelemetry SIG?

The main concern is that plugin-based sampling affects data collection in a transparent fashion, and could lead to users creating an incoherent configuration which results in data loss or inaccurate analysis. So we want to ensure that all of the sampling features available in OpenTelemetry are designed to be coherent for the broader set of features OpenTelemetry provides, such as metrics and exemplars. Jaeger is the only backend that OpenTelemetry is committed to supporting which is doing work in this field, so I just want to confirm that you all are fine with coordinating on this work going forwards via an OpenTelemetry SIG.

yurishkuro · 2021-07-22T15:59:01Z

@tedsuo sgtm

jpkrohling · 2021-07-22T18:24:29Z

Green light from me as well.

pavolloffay · 2021-07-22T19:56:05Z

@pavolloffay @yurishkuro @jpkrohling for future sampling work, is the Jaeger community comfortable with specifying those mechanisms as part of an OpenTelemetry SIG?

In Jaeger community we don't have any plans on enhancing the remote sampler, in fact we want to sunset our clients/SDKs in favor of OTEL.

pavolloffay · 2021-07-23T13:10:51Z

@tedsuo

First, can we get a specification for the current Jaeger remote sampling format and protocol OpenTelemetry is expected to support via a sampling plugin? Either written here as a spec, or linked to in the Jaeger documentation?

The text already has a link to jaeger sampling spec (proto file). Is that enough?

specification/sdk-environment-variables.md

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

pavolloffay · 2021-07-28T15:39:17Z

What is the process to get this merged? It has been approved by a couple of people. I would like to start working on the implementation.

* Add jaeger remote sampler to SDK configuration Signed-off-by: Pavol Loffay <[email protected]> * Use = Signed-off-by: Pavol Loffay <[email protected]> * add title back Signed-off-by: Pavol Loffay <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

pavolloffay requested review from a team July 5, 2021 09:40

github-actions bot assigned arminru Jul 5, 2021

pavolloffay commented Jul 5, 2021

View reviewed changes

pavolloffay mentioned this pull request Jul 5, 2021

Make Jaeger remote sampler available via SPI open-telemetry/opentelemetry-java#3368

Merged

anuraaga reviewed Jul 5, 2021

View reviewed changes

yurishkuro reviewed Jul 5, 2021

View reviewed changes

pavolloffay force-pushed the jaeger-remote-sampler branch from 1dcdc6f to 7242eb1 Compare July 6, 2021 10:13

carlosalberto added area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Jul 6, 2021

yurishkuro reviewed Jul 6, 2021

View reviewed changes

anuraaga reviewed Jul 6, 2021

View reviewed changes

yurishkuro approved these changes Jul 7, 2021

View reviewed changes

pavolloffay added 2 commits July 8, 2021 18:24

Add jaeger remote sampler to SDK configuration

35f5081

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

Use =

92b740c

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

pavolloffay force-pushed the jaeger-remote-sampler branch from 1a043b1 to 92b740c Compare July 8, 2021 16:24

anuraaga approved these changes Jul 13, 2021

View reviewed changes

yurishkuro reviewed Jul 20, 2021

View reviewed changes

specification/sdk-environment-variables.md Show resolved Hide resolved

jmacd mentioned this pull request Jul 22, 2021

Sampling SIG progress report #1819

Closed

jmacd approved these changes Jul 22, 2021

View reviewed changes

carlosalberto approved these changes Jul 23, 2021

View reviewed changes

yurishkuro reviewed Jul 23, 2021

View reviewed changes

specification/sdk-environment-variables.md Show resolved Hide resolved

add title back

3f5d467

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

Merge branch 'main' into jaeger-remote-sampler

d1e6b31

yurishkuro merged commit 30c3881 into open-telemetry:main Jul 28, 2021

jtescher mentioned this pull request Jul 31, 2021

Add jaeger remote sampler to SDK configuration open-telemetry/opentelemetry-rust#606

Closed

yvrhdn mentioned this pull request Aug 4, 2021

Add Jaeger remote sampler open-telemetry/opentelemetry-go-contrib#936

Merged

willarmiros mentioned this pull request Aug 11, 2021

Add remote sampler spec open-telemetry/oteps#167

Closed

pavolloffay mentioned this pull request Dec 14, 2021

Add JaegerRemoteSampler spec #2222

Merged

ahayworth mentioned this pull request May 11, 2022

Support jaeger_remote sampling strategy open-telemetry/opentelemetry-ruby#1252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jaeger remote sampler to SDK configuration #1791

Add jaeger remote sampler to SDK configuration #1791

pavolloffay commented Jul 5, 2021

pavolloffay Jul 5, 2021

anuraaga Jul 5, 2021

pavolloffay Jul 5, 2021

pavolloffay Jul 5, 2021

yurishkuro Jul 5, 2021 •

edited

Loading

pavolloffay Jul 5, 2021 •

edited

Loading

pavolloffay commented Jul 6, 2021

yurishkuro Jul 6, 2021

pavolloffay Jul 6, 2021

anuraaga Jul 6, 2021

pavolloffay Jul 6, 2021

MSNev Jul 22, 2021 •

edited

Loading

yurishkuro Jul 23, 2021

MSNev Jul 23, 2021 •

edited

Loading

yurishkuro Jul 23, 2021

yurishkuro Jul 27, 2021 •

edited

Loading

pavolloffay commented Jul 8, 2021

tedsuo commented Jul 22, 2021

yurishkuro commented Jul 22, 2021

jpkrohling commented Jul 22, 2021

pavolloffay commented Jul 22, 2021

pavolloffay commented Jul 23, 2021

pavolloffay commented Jul 28, 2021

Add jaeger remote sampler to SDK configuration #1791

Add jaeger remote sampler to SDK configuration #1791

Conversation

pavolloffay commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

pavolloffay Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

pavolloffay commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Available custom samplers

Jaeger Remote

Choose a reason for hiding this comment

yurishkuro Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

pavolloffay commented Jul 8, 2021

tedsuo commented Jul 22, 2021

yurishkuro commented Jul 22, 2021

jpkrohling commented Jul 22, 2021

pavolloffay commented Jul 22, 2021

pavolloffay commented Jul 23, 2021

pavolloffay commented Jul 28, 2021

yurishkuro Jul 5, 2021 •

edited

Loading

pavolloffay Jul 5, 2021 •

edited

Loading

MSNev Jul 22, 2021 •

edited

Loading

MSNev Jul 23, 2021 •

edited

Loading

yurishkuro Jul 27, 2021 •

edited

Loading