[ES][v1] Change the DB Tag value from `string` to `any` type #6998

Manik2708 · 2025-04-07T22:57:08Z

Which problem is this PR solving?

Discussed in [ES][v2] Change the DB Tag value from string to any type #6994

Description of the changes

We shouldn't be storing values as string in ES rather they should be stored raw but due to some unkown reason they were being stored as string so now backward compatibility should be considered for the old data and new data should be stored as raw

How was this change tested?

Unit and E2E Tests

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: Manik2708 <[email protected]>

Manik2708 · 2025-04-07T23:01:02Z

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

@@ -16,11 +16,12 @@ import (
 	"github.com/jaegertracing/jaeger/internal/storage/elasticsearch/dbmodel"
 )

-func CompareJSONSpans(t *testing.T, expected *dbmodel.Span, actual *dbmodel.Span) {
-	sortJSONSpan(expected)
-	sortJSONSpan(actual)


I don't know the reason of testing JSON Spans this way, but this way was leading to a problem that JSON decode treats every number as float64 which was failing the tests. Therefore we shouldn't be decoding and then asserting rather it should be encoding the expected span and then asserting!

I think if you're getting differences there may be something wrong with the translation. Despite the name "JSON" the function CompareJSONSpans was comparing two db spans as Go objects. After your change this function is no different than testJSONEncoding

I don't think so as the problem lies in unmarshaling, the expected span is unmarshalled from the fixture data, when we change the values from string to any in fixtures (that is change "12345" to 12345) then it change it's type to float64 from int64 so in test the expected span expects float64 everywhere where it should have expected int64. Earlier it was not a problem because every value was a string!

fair, but now this function is no different from testJSONEncoding

Should we remove it?

@yurishkuro Can we remove it? Or should we employ some other strategy like after marshalling converting all the int back to int64 from float64? These 2 PRs are blocker for embedding tagDotReplacement so can you please provide an overview over this?

Manik2708 · 2025-04-07T23:05:15Z

Integration tests have been passed and unit tests are passing locally, can we re-run them in CI?

codecov · 2025-04-07T23:05:53Z

Codecov Report

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

Project coverage is 95.99%. Comparing base (180cdfb) to head (c5e4193).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...al/storage/v1/elasticsearch/spanstore/to_domain.go	97.67%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6998      +/-   ##
==========================================
+ Coverage   95.97%   95.99%   +0.02%     
==========================================
  Files         346      346              
  Lines       20429    20474      +45     
==========================================
+ Hits        19606    19655      +49     
+ Misses        620      618       -2     
+ Partials      203      201       -2

Flag	Coverage Δ
badger_v1	`10.48% <0.00%> (-0.05%)`	⬇️
badger_v2	`2.17% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v1-manual	`15.77% <0.00%> (-0.08%)`	⬇️
cassandra-4.x-v2-auto	`2.16% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v2-manual	`2.16% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v1-manual	`15.77% <0.00%> (-0.08%)`	⬇️
cassandra-5.x-v2-auto	`2.16% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v2-manual	`2.16% <0.00%> (-0.02%)`	⬇️
elasticsearch-6.x-v1	`20.87% <63.46%> (+0.11%)`	⬆️
elasticsearch-7.x-v1	`20.95% <63.46%> (+0.11%)`	⬆️
elasticsearch-8.x-v1	`21.13% <63.46%> (+0.11%)`	⬆️
elasticsearch-8.x-v2	`2.17% <0.00%> (-0.02%)`	⬇️
grpc_v1	`11.56% <0.00%> (-0.06%)`	⬇️
grpc_v2	`8.43% <0.00%> (-0.05%)`	⬇️
kafka-3.x-v1	`10.77% <0.00%> (-0.06%)`	⬇️
kafka-3.x-v2	`2.17% <0.00%> (-0.02%)`	⬇️
memory_v2	`2.17% <0.00%> (-0.02%)`	⬇️
opensearch-1.x-v1	`21.00% <63.46%> (+0.11%)`	⬆️
opensearch-2.x-v1	`21.00% <63.46%> (+0.11%)`	⬆️
opensearch-2.x-v2	`2.17% <0.00%> (-0.02%)`	⬇️
tailsampling-processor	`0.59% <0.00%> (-0.01%)`	⬇️
unittests	`94.79% <98.07%> (+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.

yurishkuro

there are a lot of untested/uncovered code paths, can we not add table driven tests to capture them?

Manik2708 · 2025-04-08T02:34:31Z

there are a lot of untested/uncovered code paths, can we not add table driven tests to capture them?

@yurishkuro Unit Tests have failed due to unknown reason, can you please re-run them in CI to see the real code coverage?

…racing#6998) ## Which problem is this PR solving? - Discussed in jaegertracing#6994 ## Description of the changes - We shouldn't be storing values as string in ES rather they should be stored raw but due to some unkown reason they were being stored as string so now backward compatibility should be considered for the old data and new data should be stored as raw ## How was this change tested? - Unit and E2E Tests ## 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: Manik2708 <[email protected]>

…racing#6998) ## Which problem is this PR solving? - Discussed in jaegertracing#6994 ## Description of the changes - We shouldn't be storing values as string in ES rather they should be stored raw but due to some unkown reason they were being stored as string so now backward compatibility should be considered for the old data and new data should be stored as raw ## How was this change tested? - Unit and E2E Tests ## 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: Manik2708 <[email protected]> Signed-off-by: amol-verma-allen <[email protected]>

Manik2708 added 2 commits April 8, 2025 03:55

[ES][v1] Change the DB Tag value from string to any type

2950d1e

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

codecov

e01b9f1

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

Manik2708 requested a review from a team as a code owner April 7, 2025 22:57

Manik2708 requested a review from pavolloffay April 7, 2025 22:57

Merge branch 'main' into tag-v1

1940b1b

dosubot bot added the storage/elasticsearch label Apr 7, 2025

Manik2708 commented Apr 7, 2025

View reviewed changes

Manik2708 mentioned this pull request Apr 7, 2025

[ES][v2] Change the DB Tag value from string to any type #6994

Merged

4 tasks

yurishkuro reviewed Apr 7, 2025

View reviewed changes

Merge branch 'main' into tag-v1

c5e4193

Manik2708 requested a review from yurishkuro April 8, 2025 17:57

yurishkuro approved these changes Apr 10, 2025

View reviewed changes

yurishkuro added the changelog:bugfix-or-minor-feature label Apr 10, 2025

yurishkuro enabled auto-merge April 10, 2025 02:30

yurishkuro added this pull request to the merge queue Apr 10, 2025

Merged via the queue into jaegertracing:main with commit 1eac31e Apr 10, 2025
56 of 57 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ES][v1] Change the DB Tag value from `string` to `any` type #6998

[ES][v1] Change the DB Tag value from `string` to `any` type #6998

Uh oh!

Manik2708 commented Apr 7, 2025

Uh oh!

Manik2708 Apr 7, 2025

Uh oh!

yurishkuro Apr 9, 2025

Uh oh!

Manik2708 Apr 9, 2025

Uh oh!

yurishkuro Apr 9, 2025

Uh oh!

Manik2708 Apr 9, 2025

Uh oh!

Manik2708 Apr 9, 2025

Uh oh!

Manik2708 commented Apr 7, 2025

Uh oh!

codecov bot commented Apr 7, 2025 •

edited

Loading

Uh oh!

yurishkuro left a comment

Uh oh!

Manik2708 commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

[ES][v1] Change the DB Tag value from string to any type #6998

[ES][v1] Change the DB Tag value from string to any type #6998

Uh oh!

Conversation

Manik2708 commented Apr 7, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

Manik2708 Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

yurishkuro Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Manik2708 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

yurishkuro Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Manik2708 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Manik2708 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Manik2708 commented Apr 7, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

Manik2708 commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

[ES][v1] Change the DB Tag value from `string` to `any` type #6998

[ES][v1] Change the DB Tag value from `string` to `any` type #6998

codecov bot commented Apr 7, 2025 •

edited

Loading