[ES] Make NestedTags and FieldTags distinction at CoreSpanWriter level

[Manik2708]

Which problem is this PR solving?

• Fixes a part of: Upgrade Storage Backends to V2 Storage API #6458

Description of the changes

• TagDotReplacement which is responsible for nested and field tags distinction was earlier at writer level (of model and OTLP) which doesn't make sense as DB layer should manipulate whatever is optimal for the database, therfore now it is a part of CoreSpanWriter

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.01%. Comparing base (e22fa5d) to head (19c4772).
Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6946   +/-   ##
=======================================
  Coverage   96.01%   96.01%           
=======================================
  Files         355      355           
  Lines       20993    21012   +19     
=======================================
+ Hits        20156    20175   +19     
  Misses        630      630           
  Partials      207      207           

Flag	Coverage Δ
badger_v1	9.94% <0.00%> (-0.02%)	⬇️
badger_v2	2.06% <0.00%> (-0.01%)	⬇️
cassandra-4.x-v1-manual	14.96% <0.00%> (-0.03%)	⬇️
cassandra-4.x-v2-auto	2.05% <0.00%> (-0.01%)	⬇️
cassandra-4.x-v2-manual	2.05% <0.00%> (-0.01%)	⬇️
cassandra-5.x-v1-manual	14.96% <0.00%> (-0.03%)	⬇️
cassandra-5.x-v2-auto	2.05% <0.00%> (-0.01%)	⬇️
cassandra-5.x-v2-manual	2.05% <0.00%> (-0.01%)	⬇️
elasticsearch-6.x-v1	20.05% <94.73%> (+0.12%)	⬆️
elasticsearch-7.x-v1	20.13% <94.73%> (+0.12%)	⬆️
elasticsearch-8.x-v1	20.30% <94.73%> (+0.12%)	⬆️
elasticsearch-8.x-v2	2.06% <0.00%> (-0.01%)	⬇️
grpc_v1	11.49% <0.00%> (-0.03%)	⬇️
grpc_v2	2.06% <0.00%> (-0.01%)	⬇️
kafka-3.x-v1	10.22% <0.00%> (-0.02%)	⬇️
kafka-3.x-v2	2.06% <0.00%> (-0.01%)	⬇️
memory_v2	2.06% <0.00%> (-0.01%)	⬇️
opensearch-1.x-v1	20.18% <94.73%> (+0.12%)	⬆️
opensearch-2.x-v1	20.18% <94.73%> (+0.12%)	⬆️
opensearch-2.x-v2	2.06% <0.00%> (-0.01%)	⬇️
query	2.06% <0.00%> (-0.01%)	⬇️
tailsampling-processor	0.56% <0.00%> (-0.01%)	⬇️
unittests	94.79% <98.24%> (+<0.01%)	⬆️

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.

[Manik2708]

@yurishkuro This is a try to separate tag appending from the ToDBModel and embed tagDotReplacement in the ToDBModel. Please review!

[yurishkuro]

this is the example where snapshot testing would be very helpful, maybe spend a day to build those tests first.

[Manik2708]

this is the example where snapshot testing would be very helpful, maybe spend a day to build those tests first.

@yurishkuro I am able to generate or reuse the db spans fixtures but I don't have any idea to serialize-deserialize ptrace from json fixtures. Is there anything provided by OTEL? Or we need to use txt fixtures which are used in goldendataset
I have tested the already existing fixture in the following way:

func TestToDbModel_Fixtures(t *testing.T) {
	dbStr, err := os.ReadFile("../../../elasticsearch/dbmodel/fixtures/es_01.json")
	require.NoError(t, err)
	var span dbmodel.Span
	err = json.Unmarshal(dbStr, &span)
	require.NoError(t, err)
	td, err := FromDBModel([]dbmodel.Span{span})
	require.NoError(t, err)
	spans := ToDBModel(td)
	assert.Equal(t, span, spans[0])
}

[Manik2708]

@yurishkuro Please review this PR, after this PR, similar modification will be done in FromDBModel and we will get rid of es_01.json

[Manik2708]

@yurishkuro Can you please review this PR as it is a blocker for the next PR (embedding tagDotReplacement in FromDBModel)

[Manik2708]

#6946 (comment) @yurishkuro There is a problem in this approach, if we will firstly convert all the tags to db tag and then materialize them to top-level fields then the value of dbmodel.KeyValue is always put as string (this is what is being done in v1) whereas in tag map it can be anything. For example when 25 is saved in dbmodel.KeyValue it is stored as "25" in tags but is stored as 25 in tag map (I did this here because we don't want to change the db level span in v2). If we will employ this approach then we will need to convert strings back to their values to put into tag map.

[yurishkuro]

in the DB model we have Tags []KeyValue and

type KeyValue struct {
        Key   string    `json:"key"`
        Type  ValueType `json:"type,omitempty"`
        Value any       `json:"value"`
}

The Value here is not string, similar to Tag map[string]any. So I don't see a discrepancy - materializing Tags -> Tag should be lossless.

[Manik2708]

in the DB model we have Tags []KeyValue and

type KeyValue struct {
        Key   string    `json:"key"`
        Type  ValueType `json:"type,omitempty"`
        Value any       `json:"value"`
}

The Value here is not string, similar to Tag map[string]any. So I don't see a discrepancy - materializing Tags -> Tag should be lossless.

@yurishkuro That exactly what even I thought but in v1, every value is string in snapshot tests, also please see this:

jaeger/internal/storage/v1/elasticsearch/spanstore/from_domain.go

Line 125 in 81c9ed9

Value: kv.AsString(),

Every value which is stored in db tag is string but in tag map, it is like this:

jaeger/internal/storage/v1/elasticsearch/spanstore/from_domain.go

Line 86 in 81c9ed9

tagsMap[strings.ReplaceAll(kv.Key, ".", fd.tagDotReplacement)] = kv.Value()

So how should we proceed in v2? As we have to think of backward-compatibilty also as when converting db spans back to OTEL traces, we have to think whether the value is string or any!

[yurishkuro]

I don't know why v1 converter uses AsString(), I believe I even added a TODO asking about that specifically. But we don't have to blindly replicate v1 converter behavior, I think it should be capturing Value() regardless of where the tag is stored. It's especially important for numeric fields as storing raw value means ES queries can be made against such numeric field, e.g. computing some stats.

The only price we'd pay for using Value() is that the reverse translation (db->otlp) will have to be able to deal with the stored value being either a raw value or a string.

BTW, using Value() directly is not always correct because if the type is Binary it should be encoded somehow. There may be other limitations of using raw values, e.g. whole numbers in JS are limited to 53bits, so we already have some special handing for those when returning to UI to avoid losing precision.

[Manik2708]

@yurishkuro Is this PR good to go?

[Manik2708]

@yurishkuro Is this PR good to go now?