Update Elasticsearch to use olivere/elastic/v7 #7244

pipiland2612 · 2025-06-21T19:02:47Z

Which problem is this PR solving?

Supporting the PR [spm] GetCallRate implementation #7229

Description of the changes

update elasticsearch to use elastic/v7

How was this change tested?

make lint test

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

yurishkuro · 2025-06-21T22:00:49Z

We already ignore the leaking parts of the code from ES in internal/testutils/leakcheck.go

Is there another candidate we need to add?

internal/storage/elasticsearch/wrapper/wrapper.go

pipiland2612 · 2025-06-23T17:07:27Z

Hmm, why the CI keeps failing ?

yurishkuro · 2025-06-23T18:34:10Z

Hmm, why the CI keeps failing ?

Please stamp #7247 so that the logs are better for troubleshooting.

yurishkuro · 2025-06-23T18:59:59Z

rerunning with better logging

yurishkuro · 2025-06-23T19:10:58Z

The error is still there

        Messages:   	loading large trace, expected=10008, actual=10000
    trace_compare.go:66: Removed spans with duplicate span IDs; before=10000, after=9900

I don't recall what's the deal with these mismatching counts, can you look into what the test is doing?

In retrospect I think it would've been better to separate large trace test with a test that contains spans with duplicate span IDs (the latter could be on a much smaller trace).

pipiland2612 · 2025-06-23T19:23:14Z

Hi, it's actually that the tests is trying to verify two behaviors at once:

Ability to handle large traces
Behavior with duplicate span IDs

yurishkuro · 2025-06-23T19:34:48Z

the test is trying to verify two behaviors at once

Yes, and we should split that (want to create a PR?) But what I don't understand is the different numbers. The test is generating 10008 spans (intentionally more than the default 10k limit in some DBs), but it's also creating dups every 100 spans (for a total of 100). So if dups are removed I would expect the total to be 9908, not round 9900. Unfortunately, we don't seem to log these numbers in the successful runs, to compare with your diff.

yurishkuro · 2025-06-23T19:38:47Z

Messages: loading large trace, expected=10008, actual=10000

Actually, I think this is precisely the issue and could be related to your changes - despite duplicate span IDs we still expect to load all spans from storage, and only later to dedupe then. But this new log line says we were only able to load exactly 10k spans, which is the ES default doc limit which we were somehow working around (possible via this total hits count).

pipiland2612 · 2025-06-23T19:39:42Z

Yeah, I think it's the problem

pipiland2612 · 2025-06-23T19:47:07Z

Yes, and we should split that (want to create a PR?)

Sure I can make pull request to solve this tomorrow

pipiland2612 · 2025-06-23T20:00:45Z

spans, which is the ES default doc limit which we were somehow working around (possible via this total hits count).

Do you think we should modify the test to write 10k spans instead of 10k + 8 ?

yurishkuro · 2025-06-23T20:26:00Z

absolutely not, it's intentionally writing >10k. If it was only writing 10k then your change would've passed and we wouldn't catch the bug.

Signed-off-by: pipiland2612 <[email protected]>

Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: pipiland2612 <[email protected]>

Signed-off-by: pipiland2612 <[email protected]>

codecov · 2025-06-24T07:31:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.20%. Comparing base (108d34c) to head (80e2261).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7244      +/-   ##
==========================================
+ Coverage   96.18%   96.20%   +0.02%     
==========================================
  Files         369      369              
  Lines       22184    22181       -3     
==========================================
+ Hits        21338    21340       +2     
+ Misses        632      628       -4     
+ Partials      214      213       -1

Flag	Coverage Δ
badger_v1	`9.85% <0.00%> (+<0.01%)`	⬆️
badger_v2	`1.89% <0.00%> (+<0.01%)`	⬆️
cassandra-4.x-v1-manual	`14.81% <0.00%> (+<0.01%)`	⬆️
cassandra-4.x-v2-auto	`1.88% <0.00%> (+<0.01%)`	⬆️
cassandra-4.x-v2-manual	`1.88% <0.00%> (+<0.01%)`	⬆️
cassandra-5.x-v1-manual	`14.81% <0.00%> (+<0.01%)`	⬆️
cassandra-5.x-v2-auto	`1.88% <0.00%> (+<0.01%)`	⬆️
cassandra-5.x-v2-manual	`1.88% <0.00%> (+<0.01%)`	⬆️
elasticsearch-6.x-v1	`20.78% <57.14%> (+<0.01%)`	⬆️
elasticsearch-7.x-v1	`20.83% <57.14%> (-0.03%)`	⬇️
elasticsearch-8.x-v1	`21.01% <57.14%> (-0.03%)`	⬇️
elasticsearch-8.x-v2	`1.89% <0.00%> (+<0.01%)`	⬆️
grpc_v1	`11.39% <0.00%> (+<0.01%)`	⬆️
grpc_v2	`1.89% <0.00%> (+<0.01%)`	⬆️
kafka-3.x-v1	`10.21% <0.00%> (+<0.01%)`	⬆️
kafka-3.x-v2	`1.89% <0.00%> (+<0.01%)`	⬆️
memory_v2	`1.89% <0.00%> (+<0.01%)`	⬆️
opensearch-1.x-v1	`20.88% <57.14%> (-0.03%)`	⬇️
opensearch-2.x-v1	`20.88% <57.14%> (-0.03%)`	⬇️
opensearch-2.x-v2	`1.89% <0.00%> (+<0.01%)`	⬆️
query	`1.89% <0.00%> (+<0.01%)`	⬆️
tailsampling-processor	`0.52% <0.00%> (+<0.01%)`	⬆️
unittests	`95.05% <100.00%> (+0.03%)`	⬆️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pipiland2612 · 2025-06-24T07:49:13Z

internal/storage/v1/elasticsearch/spanstore/reader.go


-			s := s.sourceFn(query, nextTime)
+			s := s.sourceFn(query, nextTime).
+				TrackTotalHits(true)


// TotalHits is a convenience function to return the number of hits for
// a search result. The return value might not be accurate, unless
// track_total_hits parameter has set to true.

The totalhits function has this comment. So I enable it

Signed-off-by: pipiland <[email protected]>

pipiland2612 · 2025-06-24T16:36:10Z

hi @yurishkuro, the pr is ready for your review

internal/storage/v1/elasticsearch/options.go

Signed-off-by: pipiland2612 <[email protected]>

#7248) ## Which problem is this PR solving? - Comment [split GetLargeSpan](#7244 (comment)) ## Description of the changes - Split GetLargeSpan test into 2 tests: GetLargeSpans and GetSpanWithDuplicate ## How was this change tested? - make lint test ## 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`: `npm run lint` and `npm run test` --------- Signed-off-by: pipiland2612 <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

pipiland2612 · 2025-06-24T19:04:01Z

Thanks for your support!

pipiland2612 requested a review from a team as a code owner June 21, 2025 19:02

pipiland2612 requested a review from albertteoh June 21, 2025 19:02

dosubot bot added the storage/elasticsearch label Jun 21, 2025

yurishkuro reviewed Jun 21, 2025

View reviewed changes

internal/storage/elasticsearch/wrapper/wrapper.go Show resolved Hide resolved

pipiland2612 force-pushed the Update_es_to_use_esv7 branch from 8c66d34 to 0321b3d Compare June 24, 2025 07:20

pipiland2612 and others added 3 commits June 24, 2025 10:22

Update elasticsearch to use elastic/v7

1b66e80

Signed-off-by: pipiland2612 <[email protected]>

[ci] Improve error logging in e2e test (jaegertracing#7247)

c9bd446

Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: pipiland2612 <[email protected]>

Add track total hits

7a7ac74

Signed-off-by: pipiland2612 <[email protected]>

pipiland2612 force-pushed the Update_es_to_use_esv7 branch from 0321b3d to 7a7ac74 Compare June 24, 2025 07:23

pipiland2612 commented Jun 24, 2025

View reviewed changes

pipiland2612 requested a review from yurishkuro June 24, 2025 07:58

pipiland2612 mentioned this pull request Jun 24, 2025

[e2e test] Split getLargeTrace test into "large" and "with duplicates" #7248

Merged

4 tasks

Merge branch 'main' into Update_es_to_use_esv7

47937c9

Signed-off-by: pipiland <[email protected]>

yurishkuro added the changelog:bugfix-or-minor-feature label Jun 24, 2025

yurishkuro changed the title ~~Update elasticsearch to use elastic/v7~~ Update Elasticsearch to use olivere/elastic/v7 Jun 24, 2025

yurishkuro reviewed Jun 24, 2025

View reviewed changes

internal/storage/v1/elasticsearch/options.go Outdated Show resolved Hide resolved

Remove .Configuration

d6ec319

Signed-off-by: pipiland2612 <[email protected]>

pipiland2612 force-pushed the Update_es_to_use_esv7 branch from 7f7122c to d6ec319 Compare June 24, 2025 17:31

pipiland2612 requested a review from yurishkuro June 24, 2025 18:40

Merge branch 'main' into Update_es_to_use_esv7

80e2261

yurishkuro approved these changes Jun 24, 2025

View reviewed changes

yurishkuro enabled auto-merge June 24, 2025 18:58

yurishkuro added this pull request to the merge queue Jun 24, 2025

Merged via the queue into jaegertracing:main with commit 5b2b1d2 Jun 24, 2025
60 of 61 checks passed

pipiland2612 deleted the Update_es_to_use_esv7 branch June 24, 2025 19:19

pipiland2612 mentioned this pull request Jun 24, 2025

[spm] GetCallRate implementation #7229

Merged

4 tasks

Update Elasticsearch to use olivere/elastic/v7 #7244

Update Elasticsearch to use olivere/elastic/v7 #7244

Uh oh!

Conversation

pipiland2612 commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

yurishkuro commented Jun 21, 2025

Uh oh!

Uh oh!

pipiland2612 commented Jun 23, 2025

Uh oh!

yurishkuro commented Jun 23, 2025

Uh oh!

yurishkuro commented Jun 23, 2025

Uh oh!

yurishkuro commented Jun 23, 2025

Uh oh!

pipiland2612 commented Jun 23, 2025

Uh oh!

yurishkuro commented Jun 23, 2025

Uh oh!

yurishkuro commented Jun 23, 2025

Uh oh!

pipiland2612 commented Jun 23, 2025

Uh oh!

pipiland2612 commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pipiland2612 commented Jun 23, 2025

Uh oh!

yurishkuro commented Jun 23, 2025

Uh oh!

codecov bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pipiland2612 Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

pipiland2612 commented Jun 24, 2025

Uh oh!

Uh oh!

pipiland2612 commented Jun 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pipiland2612 commented Jun 21, 2025 •

edited

Loading

pipiland2612 commented Jun 23, 2025 •

edited

Loading

codecov bot commented Jun 24, 2025 •

edited

Loading