search

jaegertracing / jaeger

CNCF Jaeger, a Distributed Tracing Platform
https://www.jaegertracing.io/
Apache License 2.0
20.54k stars 2.44k forks source link

[WIP] Refactor storage factories to hold one configuration #6156

Open mahadzaryab1 opened 2 weeks ago

mahadzaryab1 commented 2 weeks ago

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

mahadzaryab1 commented 2 weeks ago

@yurishkuro we've got 3 callsites for InitArchiveStorage that currently does the runtime cast. I had a couple of questions on how to move forward here and wanted to get your thoughts.

  1. https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/extension/jaegerquery/server.go#L137-L139. The previous check to get the traces archive factory should be sufficient. If it exists, we can set the archive span reader and archive span writer in query options. Does that make sense?
  2. https://github.com/jaegertracing/jaeger/blob/772d84c3976c8ee688043f0a052d7198de30053e/cmd/query/app/flags.go#L139. This is called by cmd/query and cmd/all-in-one. Should we instantiate a new storage factory here that's the archive storage factory and pass that down to the function linked.
  3. https://github.com/jaegertracing/jaeger/blob/main/cmd/remote-storage/app/server.go#L81. This is used in the remote storage in cmd/remote-storage. Should we do the same thing here as suggested in 2?
yurishkuro commented 2 weeks ago

I don't think you need to change (1), but we need to change InitArchiveStorage() not to cast but to use the storage directly

(2) yes

(3) I think you don't need to change it because if the caller needs an archive storage it should instantiate a different remote storage. As I understand it there's not a dedicated gRPC API for archive storage, which could go away the same way as ArchiveFactory.

mahadzaryab1 commented 2 weeks ago

@yurishkuro Got it. For (2) - this is how the primary storage factory is initialized. How would we go about initializing the archive storage factory here to expose the CLI flags for storages that have archive flags (cassandra/es)

mahadzaryab1 commented 2 weeks ago

@yurishkuro For v1, what do you think of passing the isArchive flag into https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/factory.go#L116. This way, we can create a new archive storage using NewFactory, which we can pass down to es.NewFactory() and any other storage configs that need it.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.44%. Comparing base (0a24f6d) to head (4194aab). Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
plugin/storage/es/factory.go 97.56% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6156 +/- ## ========================================== - Coverage 96.47% 96.44% -0.04% ========================================== Files 354 354 Lines 20126 19997 -129 ========================================== - Hits 19417 19286 -131 - Misses 524 526 +2 Partials 185 185 ``` | [Flag](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | Coverage Δ | | |---|---|---| | [badger_v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `8.42% <0.00%> (+0.10%)` | :arrow_up: | | [badger_v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `1.70% <0.00%> (+0.02%)` | :arrow_up: | | [cassandra-4.x-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `14.15% <35.00%> (-0.24%)` | :arrow_down: | | [cassandra-4.x-v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `1.64% <0.00%> (+0.02%)` | :arrow_up: | | [cassandra-5.x-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `14.15% <35.00%> (-0.24%)` | :arrow_down: | | [cassandra-5.x-v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `1.64% <0.00%> (+0.02%)` | :arrow_up: | | [elasticsearch-6.x-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `18.18% <38.75%> (-0.43%)` | :arrow_down: | | [elasticsearch-7.x-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `18.26% <38.75%> (-0.43%)` | :arrow_down: | | [elasticsearch-8.x-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `18.43% <38.75%> (-0.42%)` | :arrow_down: | | [elasticsearch-8.x-v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `1.69% <0.00%> (+0.01%)` | :arrow_up: | | [grpc_v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `?` | | | [grpc_v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `6.91% <3.75%> (-0.09%)` | :arrow_down: | | [kafka-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `8.99% <0.00%> (+0.11%)` | :arrow_up: | | [kafka-v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `1.70% <0.00%> (+0.02%)` | :arrow_up: | | [memory_v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `1.70% <0.00%> (+0.02%)` | :arrow_up: | | [opensearch-1.x-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `18.30% <38.75%> (-0.44%)` | :arrow_down: | | [opensearch-2.x-v1](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `18.31% <38.75%> (-0.43%)` | :arrow_down: | | [opensearch-2.x-v2](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `1.69% <0.00%> (+0.01%)` | :arrow_up: | | [tailsampling-processor](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `0.47% <0.00%> (+<0.01%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/jaegertracing/jaeger/pull/6156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing) | `95.29% <83.75%> (-0.10%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jaegertracing#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mahadzaryab1 commented 2 weeks ago

@yurishkuro For ES Archive, it looks like the archive flag is used in two places:

For Cassandra, its a bit more straightforward:

Do you have any thoughts on how we should proceed? Do we still want to expose an isArchive or setAsArchive flag?

yurishkuro commented 2 weeks ago

The metrics namespacing for Cassandra can be easily done elsewhere, does not need to be based on isArchive. It's only needed there now because primary/archive distinction is made internally in the factory.

For ES:

mahadzaryab1 commented 2 weeks ago

The metrics namespacing for Cassandra can be easily done elsewhere, does not need to be based on isArchive. It's only needed there now because primary/archive distinction is made internally in the factory.

With the setup of v2, how would we make that distinction?

For ES:

  • "To choose the suffix for the index name" - not needed since the user can do that themselves. One significant change in v2 is that we cannot provide different defaults in the config for primary/archive, and having the same index prefix by mistake will be bad for the user. Maybe we can introduce additional validation for configs of the same type and catch that as a configuration error.

@yurishkuro Okay I see. But if the configurations are being held in different factories - how would we perform validation there?

  • "to add sorting and a search after clause if we're not querying the archive index" - I don't understand the purpose of that difference. Any ideas? Would it hurt if the logic for archive was the same as for primary?

I was thinking the same as well. I'm guessing its an optimization to avoid sorting the archive storage which would be larger than the a non-archive storage. Here is the documentation for Search After. Would we have a performance degradation here if we enabled this for archive as well?

  • There is a 3rd, most important usage of isArchive - in the GetIndicesFn. That's the one where I wonder if we could replace isArchive with a different logic based on the lookback parameter.

Ah yes. I was only looking at the reader. Are you referring to getSpanAndServiceIndexFn? This looks to once again be creating a different suffix based on whether the storage is archive or not (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/es/spanstore/writer.go#L104-L111). Can we not use the same approach here as the first point for the ES reader?

yurishkuro commented 2 weeks ago

With the setup of v2, how would we make that distinction?

the storage extension manages factories and knows storage names, it can use those names to bound MetricsFactory to have a specific label.

Okay I see. But if the configurations are being held in different factories - how would we perform validation there?

No, configuration are passed to factories, but held in a single place in storage extension, which can invoke additional validation here: https://github.com/jaegertracing/jaeger/blob/a420fd9174d0062a165072d1487e41ffe7e349f3/cmd/jaeger/internal/extension/jaegerstorage/extension.go#L118

Would we have a performance degradation here if we enabled this for archive as well?

but the thing is, we never search for traces in archive storage, we only retrieve trace by ID, so sorting in this case would only apply to the spans within a trace - yes, could have overhead for very large trace, but still small.

yurishkuro commented 2 weeks ago

Are you referring to getSpanAndServiceIndexFn? This looks to once again be creating a different suffix

not just suffix, when it's primary storage with manually rotated indices they also have the date pattern in the name, but archive index never has that (because it doesn't grow large). One compromise we could do is recommend that users don't use archive storage with manually rotated indices, only with ILM. Unless there's another way that I am not seeing.

Btw, reader also has similar branching in index naming logic: https://github.com/jaegertracing/jaeger/blob/a420fd9174d0062a165072d1487e41ffe7e349f3/plugin/storage/es/spanstore/reader.go#L174

mahadzaryab1 commented 2 weeks ago

With the setup of v2, how would we make that distinction?

the storage extension manages factories and knows storage names, it can use those names to bound MetricsFactory to have a specific label.

So this is where the Cassandra storage is initialized in the extension. Are you suggesting we can pass the label into the constructor here? If so, how would we make the distinction based on just the name?

Okay I see. But if the configurations are being held in different factories - how would we perform validation there?

No, configuration are passed to factories, but held in a single place in storage extension, which can invoke additional validation here:

Oh okay, I see. So whenever we're processing an ES config, we would go through all the other ones that exist and make sure that the index prefixes are not the same?

https://github.com/jaegertracing/jaeger/blob/a420fd9174d0062a165072d1487e41ffe7e349f3/cmd/jaeger/internal/extension/jaegerstorage/extension.go#L118

Would we have a performance degradation here if we enabled this for archive as well?

but the thing is, we never search for traces in archive storage, we only retrieve trace by ID, so sorting in this case would only apply to the spans within a trace - yes, could have overhead for very large trace, but still small.

Sounds good. I can remove the indirection here then.

mahadzaryab1 commented 2 weeks ago

Are you referring to getSpanAndServiceIndexFn? This looks to once again be creating a different suffix

not just suffix, when it's primary storage with manually rotated indices they also have the date pattern in the name, but archive index never has that (because it doesn't grow large). One compromise we could do is recommend that users don't use archive storage with manually rotated indices, only with ILM. Unless there's another way that I am not seeing.

Btw, reader also has similar branching in index naming logic:

https://github.com/jaegertracing/jaeger/blob/a420fd9174d0062a165072d1487e41ffe7e349f3/plugin/storage/es/spanstore/reader.go#L174

How would making that recommendation simplify the archive branching for us?

mahadzaryab1 commented 1 week ago

@yurishkuro Regarding getSourceFn, the es archive integration tests seem to fail when the SearchAfter clause is added as it is unable to find the trace.

yurishkuro commented 3 days ago

Directionally this LGTM. Any blockers?

mahadzaryab1 commented 3 days ago

Directionally this LGTM. Any blockers?

@yurishkuro The main blocker is that currently this implementation doesn't initialize the CLI flags for archive storage. We initialize the storage factory in https://github.com/jaegertracing/jaeger/blob/main/cmd/all-in-one/main.go#L58 which in this PR will only initialize the primary storages because the storage factories only hold one configuration. Any thoughts on how we should initialize the archive CLI flags?

yurishkuro commented 3 days ago

v1 mains need to create two factories, primary and archive, and use both when registering CLI flags. It's worth having a helper function for that in storage/, because you would call this from all-in-one, query, and remote-storage. The helper also may need to be smart about which storages today support archive mode and which do not, and return nil/noop factory for archive for the latter