[WIP][v2][query] Create archive reader/writer using regular factory methods #6519

mahadzaryab1 · 2025-01-10T02:04:40Z

Which problem is this PR solving?

Towards Phase out the distinction between primary and archive storage #6065

Description of the changes

How was this change tested?

Checklist

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

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 · 2025-01-10T02:10:57Z

@yurishkuro should this be marked as a breaking change? the es storage wont use the -archive alias anymore for jaeger-v2

codecov · 2025-01-10T02:11:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.27%. Comparing base (0b5f8b1) to head (d18a211).
Report is 3 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (0b5f8b1) and HEAD (d18a211). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (0b5f8b1) HEAD (d18a211)

unittests 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6519       +/-   ##
===========================================
- Coverage   96.25%   50.27%   -45.98%     
===========================================
  Files         372      188      -184     
  Lines       21371    11411     -9960     
===========================================
- Hits        20570     5737    -14833     
- Misses        610     5219     +4609     
- Partials      191      455      +264

Flag	Coverage Δ
badger_v1	`10.65% <ø> (ø)`
badger_v2	`2.78% <ø> (ø)`
cassandra-4.x-v1-manual	`16.55% <ø> (ø)`
cassandra-4.x-v2-auto	`2.71% <ø> (ø)`
cassandra-4.x-v2-manual	`2.71% <ø> (ø)`
cassandra-5.x-v1-manual	`16.55% <ø> (ø)`
cassandra-5.x-v2-auto	`2.71% <ø> (ø)`
cassandra-5.x-v2-manual	`2.71% <ø> (ø)`
elasticsearch-6.x-v1	`20.34% <ø> (+<0.01%)`	⬆️
elasticsearch-7.x-v1	`20.41% <ø> (ø)`
elasticsearch-8.x-v1	`20.57% <ø> (ø)`
elasticsearch-8.x-v2	`2.78% <ø> (-0.10%)`	⬇️
grpc_v1	`12.16% <ø> (-0.01%)`	⬇️
grpc_v2	`9.03% <ø> (-0.01%)`	⬇️
kafka-3.x-v1	`10.33% <ø> (ø)`
kafka-3.x-v2	`2.78% <ø> (ø)`
memory_v2	`2.78% <ø> (ø)`
opensearch-1.x-v1	`20.46% <ø> (+<0.01%)`	⬆️
opensearch-2.x-v1	`20.45% <ø> (ø)`
opensearch-2.x-v2	`2.77% <ø> (-0.01%)`	⬇️
tailsampling-processor	`0.51% <ø> (ø)`
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mahad Zaryab <[email protected]>

yurishkuro

There is a breaking change implied here, in the sense that if the users do not set the config options appropriately the new storage objects may not behave the same way as archive objects did previously. I recommend handling it with a feature gate from OTEL. We can declare the feature with Beta state (i.e. enabled right away, mark as breaking change), but the users have the option to turn it off manually and fall back onto the old behavior that we should preserve for now. Then in the next release we set the feature to Stable where turning it off will give a runtime error (could also be labeled a breaking change) so the new behavior is the only one possible. And then in the following release we remove the feature altogether along with the legacy code.

example in cmd/jaeger/internal/extension/remotesampling/config.go

yurishkuro · 2025-01-15T02:43:25Z

cmd/jaeger/internal/extension/jaegerquery/server.go

-	if ar != nil && aw != nil {
-		v2opts.ArchiveTraceReader = ar
-		v2opts.ArchiveTraceWriter = aw
+	if reader != nil && writer != nil {


isn't this always true now? regular Create methods are not allowed to return nil without error

yurishkuro

lgtm

yurishkuro · 2025-01-15T02:47:13Z

I have a feeling we will still need to address the question of removing create-archive methods later because as we start LFX project and being implementing v2 API in the storage directly, we will run into issue with Elasticsearch implementation.

We need to implement a feature gate

yurishkuro · 2025-01-15T02:51:02Z

I think this is breaking backwards compatibility, is it not? I.e. to get the equivalent behavior as the previous version the user needs to set some parameters (which ones, what combination?)

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 · 2025-01-16T03:01:18Z

Test Report

1. Establish Ground Truth on `main`

To begin, configure the storage and query settings in the all-in-one.yaml file as follows:

jaeger_storage:
  backends:
    some_storage:
      elasticsearch:
        indices:
          index_prefix: "jaeger-main"
          spans:
            date_layout: "2006-01-02"
            rollover_frequency: "day"
            shards: 5
            replicas: 1
          services:
            date_layout: "2006-01-02"
            rollover_frequency: "day"
            shards: 5
            replicas: 1
          dependencies:
            date_layout: "2006-01-02"
            rollover_frequency: "day"
            shards: 5
            replicas: 1
          sampling:
            date_layout: "2006-01-02"
            rollover_frequency: "day"
            shards: 5
            replicas: 1
    another_storage:
      elasticsearch:
        indices:
          index_prefix: "jaeger-archive"

2. Spin Up Elasticsearch Using Docker

To bring up Elasticsearch, run the following commands:

jaeger % cd docker-compose/elasticsearch/v8 
v8 % docker compose up

3. Start Jaeger

Start the Jaeger service:

jaeger % go run ./cmd/jaeger

4. Archive a Trace

From the Jaeger UI, select a trace and archive it.

traceID: 0dc3e460bd9b8e0dddfa29a2f751cfb9

5. Update `index_prefix`

Stop Jaeger and modify the index_prefix in the primary configuration to jaeger-main-1. This ensures the query for the same trace is no longer found in the primary storage but will be found in the archive storage.

6. Query for the Same Trace

curl -s http://localhost:16686/api/traces/0dc3e460bd9b8e0dddfa29a2f751cfb9 | jq '.data[].spans[] | {traceID, operationName}'
{
  "traceID": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "operationName": "/api/services"
}
{
  "traceID": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "operationName": "GetService"
}

curl -s http://localhost:16686/api/v3/traces/0dc3e460bd9b8e0dddfa29a2f751cfb9 | jq '.result.resourceSpans[].scopeSpans[].spans[] | {traceId, name}'
{
  "traceId": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "name": "GetService"
}
{
  "traceId": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "name": "/api/services"
}

7. Test Changes from This PR

Stop Jaeger, checkout this PR, and restart Jaeger:

gh pr checkout 6519
go run ./cmd/jaeger

8. Query for the Same Trace After PR Changes

curl -s http://localhost:16686/api/traces/0dc3e460bd9b8e0dddfa29a2f751cfb9 | jq .
{
  "data": null,
  "total": 0,
  "limit": 0,
  "offset": 0,
  "errors": [
    {
      "code": 404,
      "msg": "trace not found"
    }
  ]
}

curl -s http://localhost:16686/api/v3/traces/0dc3e460bd9b8e0dddfa29a2f751cfb9 | jq .
{
  "error": {
    "httpCode": 404,
    "message": "No traces found"
  }
}

🛑 This is where the breaking change occurs 🛑

9. Mitigation for Users

Set `use_aliases` for Archive Storage

To mitigate the issue, set the use_aliases configuration for your archive storage to true. Update the configuration as follows:

another_storage:
  elasticsearch:
    indices:
      index_prefix: "jaeger-archive"
    use_aliases: true

Add Alias from Old Index to New Index

To ensure backwards compatibility, add an alias from the old index to the new index. You can query the current set of aliases in Elasticsearch with:

curl -X GET "http://localhost:9200/_aliases?pretty"

{
  "jaeger-main-1-jaeger-span-2025-01-16": { "aliases": {} },
  "jaeger-main-2-jaeger-span-2025-01-16": { "aliases": {} },
  "jaeger-archive-jaeger-span-archive": { "aliases": {} },
  "jaeger-main-1-jaeger-service-2025-01-16": { "aliases": {} }
}

To link the new index (jaeger-archive-jaeger-span-read) with the old index (jaeger-archive-jaeger-span-archive), run the following:

curl -X POST "http://localhost:9200/_aliases" -H 'Content-Type: application/json' -d'
{
  "actions": [
    {
      "add": {
        "index": "jaeger-archive-jaeger-span-archive",
        "alias": "jaeger-archive-jaeger-span-read"
      }
    }
  ]
}'

Confirm that the alias has been added:

curl -X GET "http://localhost:9200/_aliases?pretty"

{
  "jaeger-main-1-jaeger-span-2025-01-16": { "aliases": {} },
  "jaeger-archive-jaeger-span-archive": { "aliases": { "jaeger-archive-jaeger-span-read": {} } },
  "jaeger-main-2-jaeger-span-2025-01-16": { "aliases": {} },
  "jaeger-main-2-jaeger-service-2025-01-16": { "aliases": {} },
  "jaeger-main-1-jaeger-service-2025-01-16": { "aliases": {} }
}

10. Restart Jaeger and Retry the Query

Finally, restart Jaeger and run the same trace queries again:

curl -s http://localhost:16686/api/traces/0dc3e460bd9b8e0dddfa29a2f751cfb9 | jq '.data[].spans[] | {traceID, operationName}'
{
  "traceID": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "operationName": "/api/services"
}
{
  "traceID": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "operationName": "GetService"
}

curl -s http://localhost:16686/api/v3/traces/0dc3e460bd9b8e0dddfa29a2f751cfb9 | jq '.result.resourceSpans[].scopeSpans[].spans[] | {traceId, name}'
{
  "traceId": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "name": "/api/services"
}
{
  "traceId": "0dc3e460bd9b8e0dddfa29a2f751cfb9",
  "name": "GetService"
}

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 and others added 2 commits January 9, 2025 21:00

Fix Tests

283552d

Signed-off-by: Mahad Zaryab <[email protected]>

Merge branch 'main' into query-archive

87a64a7

mahadzaryab1 added the changelog:bugfix-or-minor-feature label Jan 10, 2025

Add Unit Tests

399bfcb

Signed-off-by: Mahad Zaryab <[email protected]>

yurishkuro reviewed Jan 11, 2025

View reviewed changes

mahadzaryab1 mentioned this pull request Jan 11, 2025

[storage] Remove dependency on archive flag in ES reader #6490

Merged

4 tasks

Merge branch 'main' into query-archive

acad06d

yurishkuro reviewed Jan 15, 2025

View reviewed changes

yurishkuro previously approved these changes Jan 15, 2025

View reviewed changes

yurishkuro changed the title ~~[WIP][v2][query] Change jaegerquery extension to use primary reader/writer for archive storage~~ [WIP][v2][query] Create archive reader/writer using regular factory methods Jan 15, 2025

Merge branch 'main' into query-archive

2f63057

mahadzaryab1 added 2 commits January 15, 2025 20:43

Revert Initialization of Archive For v1

4090ece

Signed-off-by: Mahad Zaryab <[email protected]>

Stop Replying on InitArchiveStorage

3904944

Signed-off-by: Mahad Zaryab <[email protected]>

Add Downgrades

d18a211

Signed-off-by: Mahad Zaryab <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][v2][query] Create archive reader/writer using regular factory methods #6519

[WIP][v2][query] Create archive reader/writer using regular factory methods #6519

mahadzaryab1 commented Jan 10, 2025

mahadzaryab1 commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

yurishkuro left a comment •

edited

Loading

yurishkuro Jan 15, 2025

yurishkuro left a comment

yurishkuro commented Jan 15, 2025

yurishkuro commented Jan 15, 2025

mahadzaryab1 commented Jan 16, 2025 •

edited

Loading

[WIP][v2][query] Create archive reader/writer using regular factory methods #6519

Are you sure you want to change the base?

[WIP][v2][query] Create archive reader/writer using regular factory methods #6519

Conversation

mahadzaryab1 commented Jan 10, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

mahadzaryab1 commented Jan 10, 2025 • edited Loading

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

yurishkuro left a comment • edited Loading

Choose a reason for hiding this comment

yurishkuro Jan 15, 2025

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Jan 15, 2025

yurishkuro commented Jan 15, 2025

mahadzaryab1 commented Jan 16, 2025 • edited Loading

Test Report

1. Establish Ground Truth on main

2. Spin Up Elasticsearch Using Docker

3. Start Jaeger

4. Archive a Trace

5. Update index_prefix

6. Query for the Same Trace

7. Test Changes from This PR

8. Query for the Same Trace After PR Changes

9. Mitigation for Users

Set use_aliases for Archive Storage

Add Alias from Old Index to New Index

10. Restart Jaeger and Retry the Query

mahadzaryab1 commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

yurishkuro left a comment •

edited

Loading

mahadzaryab1 commented Jan 16, 2025 •

edited

Loading

1. Establish Ground Truth on `main`

5. Update `index_prefix`

Set `use_aliases` for Archive Storage