[ES][v2] Refactor `from_dbmodel` and `to_dbmodel` to accept and return DB Spans #6934

Manik2708 · 2025-03-26T16:00:26Z

Which problem is this PR solving?

Fixes a part of: #6458

Description of the changes

This is a prerequisite for the full implementation of converting OTLP Spans to db spans and vice-versa

How was this change tested?

Unit 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-03-26T16:02:08Z

@yurishkuro @mahadzaryab1 Please review this PR, if the changes look good, I will complete the code-coverage. Initially I thought to raise PR seperately for both files but their tests are inter-related, therefore I had to do them in a single PR.

Manik2708 · 2025-03-26T16:02:55Z

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

-	return traceData, nil
-}
-
-func regroup(batches []*model.Batch) []*model.Batch {


Grouping is removed and therefore every span has it's own resource.

Manik2708 · 2025-03-26T16:03:52Z

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

@@ -183,37 +93,71 @@ type scope struct {
 	name, version string
 }

-func jSpansToInternal(spans []*model.Span, dest ptrace.ScopeSpansSlice) {
-	spansByLibrary := make(map[scope]ptrace.SpanSlice)


We can't group spans with same scope together as before this they should have the same resource which can't be possible in our case.

Manik2708 · 2025-03-26T16:04:45Z

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

+		case dbmodel.BoolType:
+			convBoolVal, err := strconv.ParseBool(tagValue)
+			if err != nil {
+				putConversionErrKeyValuePair(tag, err, dest)


I am confused for this, whether should I return the error or put the error as an attribute in the span?

we have similar conversion implemented in convertKeyValue in internal/storage/v1/elasticsearch/spanstore/to_domain.go. It doesn't have to be much different here.

That is returning error, so we should also?

codecov · 2025-03-26T16:12:18Z

Codecov Report

Attention: Patch coverage is 98.60140% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.18%. Comparing base (26bfb4f) to head (ab91fc5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...torage/v2/elasticsearch/tracestore/from_dbmodel.go	97.27%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6934      +/-   ##
==========================================
- Coverage   96.26%   96.18%   -0.08%     
==========================================
  Files         343      344       +1     
  Lines       20313    20255      -58     
==========================================
- Hits        19554    19482      -72     
- Misses        574      583       +9     
- Partials      185      190       +5

Flag	Coverage Δ
badger_v1	`10.21% <ø> (ø)`
badger_v2	`2.12% <ø> (ø)`
cassandra-4.x-v1-manual	`15.36% <ø> (ø)`
cassandra-4.x-v2-auto	`2.11% <ø> (ø)`
cassandra-4.x-v2-manual	`2.11% <ø> (ø)`
cassandra-5.x-v1-manual	`15.36% <ø> (ø)`
cassandra-5.x-v2-auto	`2.11% <ø> (ø)`
cassandra-5.x-v2-manual	`2.11% <ø> (ø)`
elasticsearch-6.x-v1	`20.12% <ø> (ø)`
elasticsearch-7.x-v1	`20.21% <ø> (ø)`
elasticsearch-8.x-v1	`20.38% <ø> (-0.06%)`	⬇️
elasticsearch-8.x-v2	`2.12% <ø> (ø)`
grpc_v1	`11.28% <ø> (ø)`
grpc_v2	`8.23% <ø> (ø)`
kafka-3.x-v1	`10.51% <ø> (ø)`
kafka-3.x-v2	`2.12% <ø> (ø)`
memory_v2	`2.12% <ø> (ø)`
opensearch-1.x-v1	`20.26% <ø> (ø)`
opensearch-2.x-v1	`20.26% <ø> (-0.06%)`	⬇️
opensearch-2.x-v2	`2.12% <ø> (ø)`
tailsampling-processor	`0.57% <ø> (ø)`
unittests	`94.96% <98.60%> (-0.09%)`	⬇️

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.

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

yurishkuro · 2025-03-27T04:02:51Z

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

+		if !ok {
+			// We have to do this as we are unsure that whether bool will be a string or a bool
+			tagBoolVal, boolOk := tag.Value.(bool)
+			if boolOk && tag.Type == dbmodel.BoolType {
+				dest.PutBool(tag.Key, tagBoolVal)
+			} else {
+				dest.PutStr(tag.Key, "Got non string value for the key "+tag.Key)
+			}
+			continue
+		}


I don't follow what this is doing. Why does bool require different handling that all other types handled in the switch statement?

The Value of tag is any not string and in case of bool the values are in-consistent. Sometimes it is "true" and sometimes it is true, so we need to take care of both the cases!

yurishkuro

please make high level fixes suggested in the comments, its difficult to review when old/new code is all mixed up.

yurishkuro · 2025-03-27T04:26:27Z

btw, this is the equivalent for ClickHouse model, which somehow seems a lot cleaner

https://github.com/jaegertracing/jaeger/pull/6935/files

Manik2708 · 2025-03-27T05:56:46Z

btw, this is the equivalent for ClickHouse model, which somehow seems a lot cleaner

https://github.com/jaegertracing/jaeger/pull/6935/files

@yurishkuro Thanks for suggestions. When I was modifying the code, I was also concerned about code cleaniness. But then I was also worried about PR becoming excessively large. Therefore I decided to keep the scope of this PR just to replace model.Batch with []dbmodel.Span without changing any name and structure. But now I could understand that it has made this PR difficult to review. My exact plan was:

Replacing model with dbmodel (This PR)
Implementing ToModel struct with tagDotReplacement and changing the structure of to_dbmodel.go and fixing the tests (Next PR)
Implementing FromModel struct with tagDotReplacement and changing the structure of from_dbmodel.go (Next PR).

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

Manik2708 · 2025-03-29T06:31:32Z

@yurishkuro @mahadzaryab1 Please review this PR, as it is a blocker for next PRs!

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

Manik2708 · 2025-03-29T15:54:53Z

@yurishkuro Thanks for the commit! Should I work on code-coverage now?

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

yurishkuro

ok to merge this but it needs more clean-up. There is too much business logic that should not be present in the db layer. The job of db layer is to capture information without a loss, not to perform business logic translations.

yurishkuro · 2025-03-29T15:07:30Z

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

 }

-func jProcessToInternalResource(process *model.Process, dest pcommon.Resource) {
-	if process == nil || process.ServiceName == noServiceName {
+func dbProcessToResource(process dbmodel.Process, destinationResource pcommon.Resource) {


avoid unnecessary complex variable names. In this case what is even the alternative to it being "destination"?

Suggested change

func dbProcessToResource(process dbmodel.Process, destinationResource pcommon.Resource) {

func dbProcessToResource(process dbmodel.Process, resource pcommon.Resource) {

yurishkuro · 2025-03-29T15:08:44Z

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

+	if process.ServiceName == "" || process.ServiceName == noServiceName {
 		return
 	}


there's no point to this check

yurishkuro · 2025-03-29T15:11:11Z

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go

-	spansByLibrary := make(map[scope]ptrace.SpanSlice)
-
-	for _, span := range spans {
+func setSpansFromDbSpans(dbSpans []*dbmodel.Span, resourceSpans ptrace.ResourceSpansSlice) error {


use consistent naming, if you have dbProcessToResource then this should be

Suggested change

func setSpansFromDbSpans(dbSpans []*dbmodel.Span, resourceSpans ptrace.ResourceSpansSlice) error {

func dbSpansToSpans(dbSpans []*dbmodel.Span, resourceSpans ptrace.ResourceSpansSlice) error {

Manik2708 · 2025-03-29T16:37:45Z

ok to merge this but it needs more clean-up. There is too much business logic that should not be present in the db layer. The job of db layer is to capture information without a loss, not to perform business logic translations.

Ok, so should I work on the cleanup? If yes, then what should be the forwarding step for the cleanup? Also code coverage is below threshold, should I complete it in this PR? Or in a different PR?

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

yurishkuro · 2025-03-29T18:42:07Z

Ok, so should I work on the cleanup?

historically we used snapshot based testing for transformers, where you have a set of JSON fixtures used to perform round-trip conversion and to cover the edge cases. We should switch testing to that mode, e.g. by starting with the fixtures in v1 implementation and creating the corresponding OTLP files from them.

Manik2708 · 2025-03-29T18:46:16Z

Ok, so should I work on the cleanup?

historically we used snapshot based testing for transformers, where you have a set of JSON fixtures used to perform round-trip conversion and to cover the edge cases. We should switch testing to that mode, e.g. by starting with the fixtures in v1 implementation and creating the corresponding OTLP files from them.

Got it, should I work on it now or implement the writer/reader for v2?

yurishkuro · 2025-03-29T19:12:18Z

you can do both in parallel, they are now independent tasks and won't conflict with the files.

…n DB Spans (jaegertracing#6934) ## Which problem is this PR solving? Fixes a part of: jaegertracing#6458 ## Description of the changes - This is a prerequisite for the full implementation of converting OTLP Spans to db spans and vice-versa ## How was this change tested? - Unit 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: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

…n DB Spans (jaegertracing#6934) ## Which problem is this PR solving? Fixes a part of: jaegertracing#6458 ## Description of the changes - This is a prerequisite for the full implementation of converting OTLP Spans to db spans and vice-versa ## How was this change tested? - Unit 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: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: amol-verma-allen <[email protected]>

Manik2708 added 3 commits March 26, 2025 13:18

init

79488fd

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

init

dcdf3f6

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

end

701faff

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

Manik2708 requested a review from a team as a code owner March 26, 2025 16:00

Manik2708 requested a review from jkowall March 26, 2025 16:00

Merge branch 'main' into todb

8ae7385

dosubot bot added area/storage v2 labels Mar 26, 2025

Manik2708 commented Mar 26, 2025

View reviewed changes

yurishkuro reviewed Mar 27, 2025

View reviewed changes

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 27, 2025

View reviewed changes

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 27, 2025

View reviewed changes

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 27, 2025

View reviewed changes

internal/storage/v2/elasticsearch/tracestore/from_dbmodel.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 27, 2025

View reviewed changes

Manik2708 added 2 commits March 27, 2025 12:04

ids

619d672

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

Merge branch 'main' into todb

9b0ecd4

Manik2708 requested a review from yurishkuro March 27, 2025 06:37

Manik2708 added 3 commits March 28, 2025 21:28

cleanup

17d79ce

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

Merge branch 'main' into todb

36bb3ce

docs

e9dee95

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

cleanup

2fc829c

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

yurishkuro added 2 commits March 29, 2025 12:09

cleanup

77177e8

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

cleanup

e346278

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

yurishkuro approved these changes Mar 29, 2025

View reviewed changes

yurishkuro added the changelog:exprimental Change to an experimental part of the code label Mar 29, 2025

yurishkuro enabled auto-merge March 29, 2025 16:25

code-cov

ab91fc5

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

auto-merge was automatically disabled March 29, 2025 18:25
Head branch was pushed to by a user without write access

yurishkuro added this pull request to the merge queue Mar 29, 2025

Merged via the queue into jaegertracing:main with commit 97c6778 Mar 29, 2025
56 checks passed

	func dbProcessToResource(process dbmodel.Process, destinationResource pcommon.Resource) {
	func dbProcessToResource(process dbmodel.Process, resource pcommon.Resource) {

	func setSpansFromDbSpans(dbSpans []*dbmodel.Span, resourceSpans ptrace.ResourceSpansSlice) error {
	func dbSpansToSpans(dbSpans []*dbmodel.Span, resourceSpans ptrace.ResourceSpansSlice) error {

[ES][v2] Refactor from_dbmodel and to_dbmodel to accept and return DB Spans #6934

[ES][v2] Refactor from_dbmodel and to_dbmodel to accept and return DB Spans #6934

Uh oh!

Conversation

Manik2708 commented Mar 26, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

Manik2708 commented Mar 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manik2708 Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

yurishkuro commented Mar 27, 2025

Uh oh!

Manik2708 commented Mar 27, 2025

Uh oh!

Manik2708 commented Mar 29, 2025

Uh oh!

Manik2708 commented Mar 29, 2025

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manik2708 commented Mar 29, 2025

Uh oh!

yurishkuro commented Mar 29, 2025

Uh oh!

Manik2708 commented Mar 29, 2025

Uh oh!

Uh oh!

yurishkuro commented Mar 29, 2025

Uh oh!

Uh oh!

[ES][v2] Refactor `from_dbmodel` and `to_dbmodel` to accept and return DB Spans #6934

[ES][v2] Refactor `from_dbmodel` and `to_dbmodel` to accept and return DB Spans #6934

codecov bot commented Mar 26, 2025 •

edited

Loading

Manik2708 Mar 27, 2025 •

edited

Loading