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.

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

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

Closed
6 tasks done
mahadzaryab1 opened this issue Sep 25, 2024 · 8 comments
Closed
6 tasks done

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

mahadzaryab1 opened this issue Sep 25, 2024 · 8 comments

Comments

@mahadzaryab1
Copy link
Collaborator

mahadzaryab1 commented Sep 25, 2024

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 #5229)

Tasks / outcomes

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

@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
Copy link
Member

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 mahadzaryab1 changed the title [jaeger-v2] Align Adaptive Sampling Processor Config With OTEL [jaeger-v2] Refactor Adaptive Sampling Processor Configurations Sep 30, 2024
@mahadzaryab1
Copy link
Collaborator Author

mahadzaryab1 commented Oct 3, 2024

@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
Copy link
Member

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

@mahadzaryab1
Copy link
Collaborator Author

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
Copy link
Member

Feel free to propose a change.

Do we use store or storage in query service?

@mahadzaryab1
Copy link
Collaborator Author

mahadzaryab1 commented Oct 3, 2024

@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
Copy link
Collaborator Author

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.

@mahadzaryab1 mahadzaryab1 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants