[jaeger-v2] Refactor Adaptive Sampling Processor Configurations

jaegertracing / jaeger

CNCF Jaeger, a Distributed Tracing Platform

https://www.jaegertracing.io/

Apache License 2.0

20.52k stars 2.44k forks source link

[jaeger-v2] Refactor Adaptive Sampling Processor Configurations #6021

Closed mahadzaryab1 closed 1 month ago

mahadzaryab1 commented 1 month ago

Requirement

In this issue, we want to refactor the configurations for the adaptive sampling processor to ensure that the configuration is laid out logically (part of https://github.com/jaegertracing/jaeger/issues/5229)

Tasks / outcomes

[x] All config fields are tagged with mapstructure such that they can be addressable from YAML via "good" names
[x] Configs implement Validate()
[x] Configs via YAML support the same defaults as in jaeger-v1
[x] Configs reuse standard elements of OTEL configs whenever possible, e.g. TLS, clientcfg, etc.
[x] Configuration looks "logical" and properly grouped, not flattened with long weird names
[x] We create a migration guide document with a table that lists v1 CLI flags and the corresponding v2 config fields

mahadzaryab1 commented 1 month ago

@yurishkuro Do we need https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/processors/adaptivesampling/config.go? It looks to be empty and all the config seems to be in the remote sampling extension?

yurishkuro commented 1 month ago

A config is required by the overall organization of OTEL components, even if it's empty.

There's nothing to align with OTEL here since adaptive sampling is jaeger-only feature, but we can take a critical look how that config is laid out. Also, the extension runs servers, so we can make sure they are using OTEL configs and helpers.

mahadzaryab1 commented 1 month ago

@yurishkuro is there a reason that SamplingStore is separate from the rest of the config in https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/extension/remotesampling/config.go#L41-L46? Can I move SamplingStore under adaptive.Options?

yurishkuro commented 1 month ago

The reason is because it's not applicable to v1 collector.

mahadzaryab1 commented 1 month ago

Does it hurt to combine it? Currently in the config we have something like:

sampling_store:
sampling:
  ...

It would be nice to do something like

sampling:
  store:
  ...

yurishkuro commented 1 month ago

Feel free to propose a change.

Do we use store or storage in query service?

mahadzaryab1 commented 1 month ago

@yurishkuro Here is the migration guide for the adaptive sampling processor. Are there any other action items as part of this issue? If not, we can close it out.

mahadzaryab1 commented 1 month ago

It was decided that no changes were needed to the configuration. Only the migration guide was created as part of this issue which has been added to the V2 RFC.