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] Rethink components configuration to align with OTEL #5229

Open
3 of 15 tasks
yurishkuro opened this issue Feb 24, 2024 · 12 comments
Open
3 of 15 tasks

[jaeger-v2] Rethink components configuration to align with OTEL #5229

yurishkuro opened this issue Feb 24, 2024 · 12 comments
Labels

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Feb 24, 2024

Right now several PRs that are trying to add support for standard storage backends to v2 are reusing the same configuration for storage as we had in v1. Now is a good time to rethink that configuration, in particular:

  • better align with standard components of OTEL Collector. For example, the gRPC storage ([jaeger-v2] Add support for GRPC storarge #5228) is conceptually very similar to OTLP Exporter - they both send data over gRPC to a remote service. Our configuration is pretty slim, while OTLP Exporter's configuration has a lot more features, there is no reason why we cannot support them (would be even better if we could simply reuse OTEL's configuration utilities for gRPC)
  • our storage configs have been accumulating over time, we can see if there are better logical groupings. For example, ES config is a flat list of everything, we can group many flags into sub-types so that the YAML config looks a lot more structured than using long field names like priority_dependencies_template
  • ensure that we have sensible defaults. For example, in [jaeger-v2] add cassandra e2e integration tests #5398 basic options had to be added to Cassandra configuration, even though in v1 many of them are already defaulted to these values, so we're clearly missing defaulting mechanism. It's somewhat complicated by the fact that the storage extension cannot actually define the defaults in CreateDefaultConfig because at that point we don't known which storage will be used.

Scope

Tasks / outcomes

For each of the above components, we need to ensure

  • All config fields are tagged with mapstructure such that they can be addressable from YAML via "good" names
  • Configs implement Validate() -- example
  • 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 (good example [Jaeger-V2] Add configurable index data layout and rollover frequency #5790)
  • We create a migration guide document with a table that lists v1 CLI flags and the corresponding v2 config fields (example for Kafka)

How to work on this meta-issue

  • pick a component / area
  • create a dedicated ticket for it
  • copy tasks / outcomes from above
  • use the ticket for design discussions, updates, links to PRs
@pavolloffay pavolloffay added the v2 label Mar 13, 2024
yurishkuro added a commit that referenced this issue May 20, 2024
## Which problem is this PR solving?
- part of #5229 

## Description of the changes
- added more grpc storage client configuration to align with otel
 ```
	configgrpc.ClientConfig        `mapstructure:",squash"`
	exporterhelper.TimeoutSettings `mapstructure:",squash"`
 ```
These are not all the configs, but i'll add more based on feedback on
this initial approach.

## How was this change tested?
- not tested yet 

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Harshvir Potpose <[email protected]>
Signed-off-by: Harshvir Potpose <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
@mahadzaryab1
Copy link
Contributor

@yurishkuro What is the status of this issue? Is there anything I can help with?

@yurishkuro yurishkuro changed the title [jaeger-v2] Rethink storage configuration to align with OTEL [jaeger-v2] Rethink components configuration to align with OTEL Aug 31, 2024
@yurishkuro
Copy link
Member Author

yes, plenty. I just updated the description.

@mahadzaryab1
Copy link
Contributor

@yurishkuro are the configs for v1 supposed to be in plugin/storage/*/options.go and the configs for v2 supposed to be in pkg/*/config.go?

@yurishkuro
Copy link
Member Author

yes, that's accurate

@mahadzaryab1
Copy link
Contributor

mahadzaryab1 commented Sep 1, 2024

@yurishkuro how come some stores still have the v2 configuration living in plugin/storage/*? do we want to move those over?

@yurishkuro
Copy link
Member Author

We are in the transition period. Moving them doesn't solve anything, having configs side by side is actually convenient. The long term goal is to only have v2 configurations.

Wwe started v2 by just wrapping v1 configs. For some backends we did preliminary work of cleaning up and making v2 the "primary" config, i.e. the Factory uses v2 config directly, but v1 config still drives the v1 cli flags and can be transformed into v2. We could continue using this pattern. The reason we want v2 configs to be primary is to ensure that all their settings are actually used by the storage implementation - there's less guarantee of that if v1 is primary and v2 is translated into v1.

But overall this ticket is about what v2 config look like, rather than where they are located.

@mahadzaryab1
Copy link
Contributor

mahadzaryab1 commented Sep 1, 2024

@yurishkuro sounds good! and where should the migration doc be created? is any public google doc fine? Or do we want to have them in a README in this repo?

@yurishkuro
Copy link
Member Author

yes a google doc is fine

@mahadzaryab1
Copy link
Contributor

@yurishkuro PR for memory storage component at #5925

@mahadzaryab1
Copy link
Contributor

@yurishkuro PR for badger storage component at #5927

@mahadzaryab1
Copy link
Contributor

@yurishkuro PR for Cassandra component ready for review at #5949.

@mahadzaryab1
Copy link
Contributor

@yurishkuro PR for Query Service ready for review at #5998

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

No branches or pull requests

3 participants