[ES] Separate the `CoreSpanWriter` from `SpanWriter` by Manik2708 · Pull Request #6883 · jaegertracing/jaeger

Manik2708 · 2025-03-19T19:45:15Z

Which problem is this PR solving?

Fixes a part of: #6458

Description of the changes

We have to prepeare the writer to move to v2 and for that we need to separate SpanWriter

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 <mehtamanik96@gmail.com>

Manik2708 · 2025-03-19T19:46:21Z

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

 	})
 }

-func TestNewSpanTags(t *testing.T) {


This test is moved to from_domain_test.go

codecov · 2025-03-19T19:53:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.16%. Comparing base (d3f6682) to head (40360b2).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6883      +/-   ##
==========================================
- Coverage   96.17%   96.16%   -0.02%     
==========================================
  Files         341      342       +1     
  Lines       19738    19735       -3     
==========================================
- Hits        18983    18978       -5     
- Misses        571      573       +2     
  Partials      184      184

Flag	Coverage Δ
badger_v1	`10.21% <0.00%> (+<0.01%)`	⬆️
badger_v2	`2.12% <0.00%> (+<0.01%)`	⬆️
cassandra-4.x-v1-manual	`15.36% <0.00%> (+<0.01%)`	⬆️
cassandra-4.x-v2-auto	`2.11% <0.00%> (+<0.01%)`	⬆️
cassandra-4.x-v2-manual	`2.11% <0.00%> (+<0.01%)`	⬆️
cassandra-5.x-v1-manual	`15.36% <0.00%> (+<0.01%)`	⬆️
cassandra-5.x-v2-auto	`2.11% <0.00%> (+<0.01%)`	⬆️
cassandra-5.x-v2-manual	`2.11% <0.00%> (+<0.01%)`	⬆️
elasticsearch-6.x-v1	`20.12% <86.66%> (-0.03%)`	⬇️
elasticsearch-7.x-v1	`20.21% <86.66%> (-0.03%)`	⬇️
elasticsearch-8.x-v1	`20.38% <86.66%> (-0.03%)`	⬇️
elasticsearch-8.x-v2	`2.12% <0.00%> (+<0.01%)`	⬆️
grpc_v1	`11.28% <0.00%> (+<0.01%)`	⬆️
grpc_v2	`8.25% <0.00%> (+<0.01%)`	⬆️
kafka-3.x-v1	`10.51% <0.00%> (+<0.01%)`	⬆️
kafka-3.x-v2	`2.12% <0.00%> (+<0.01%)`	⬆️
memory_v2	`2.12% <0.00%> (+<0.01%)`	⬆️
opensearch-1.x-v1	`20.26% <86.66%> (-0.03%)`	⬇️
opensearch-2.x-v1	`20.26% <86.66%> (-0.03%)`	⬇️
opensearch-2.x-v2	`2.12% <0.00%> (+<0.01%)`	⬆️
tailsampling-processor	`0.57% <0.00%> (+<0.01%)`	⬆️
unittests	`94.91% <53.33%> (-0.05%)`	⬇️

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.

Signed-off-by: Manik2708 <mehtamanik96@gmail.com>

yurishkuro · 2025-03-20T00:20:56Z

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

+		spanWriter:    NewSpanWriter(p),
+		spanConverter: dbmodel.NewFromDomain(p.AllTagsAsFields, p.TagKeysAsFields, p.TagDotReplacement),
+		writerMetrics: spanWriterMetrics{
+			indexCreate: spanstoremetrics.NewWriter(p.MetricsFactory, "index_create"),


why did you pull metrics away from core writer? it's a reusable functionality

It is done to make CoreSpanWriter independent of api/v1/spanstore. If metrics factory is gonna remain same then I think we should move it to either shared pkg or v2

these metrics were used to be called from inside the business logic of the writer (last line below). So you cannot have them at the v1 writer level, then need to be internal to the core writer. In what sense is there a dependency on api/v1/spanstore? spanstoremetrics is an independent package.

func (s *SpanWriter) createIndex(indexName string, mapping string, jsonSpan *jModel.Span) error { if !keyInCache(indexName, s.indexCache) { start := time.Now() exists, _ := s.client.IndexExists(indexName).Do(s.ctx) // don't need to check the error because the exists variable will be false anyway if there is an error if !exists { // if there are multiple collectors writing to the same elasticsearch host a race condition can occur - create the index multiple times // we check for the error type to minimize errors _, err := s.client.CreateIndex(indexName).Body(s.fixMapping(mapping)).Do(s.ctx) s.writerMetrics.indexCreate.Emit(err, time.Since(start))

This is what I tried to remove:

"github.com/jaegertracing/jaeger/internal/storage/v1/api/spanstore/spanstoremetrics"

spanstoremetrics lie in spanstore

yurishkuro · 2025-03-20T01:18:56Z

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

+				Tag: map[string]any{"foo": "bar"}, Tags: []dbmodel.KeyValue{},
+				Process: dbmodel.Process{Tag: map[string]any{"bar": "baz"}, Tags: []dbmodel.KeyValue{}},
+			},
+			name: "allTagsAsFields",


which functionality is being tested the most here, core writer or v1 wrapper? I think it's core writer, and the test should stay with it. A simple way to validate - if you delete this test and the code coverage of the core writer drops, then the test is in the wrong place (because we will delete it in a year).

I now realized that, this test should be in from_domain_test.go as it is only testing FromDomainEmbedProcess

Signed-off-by: Manik2708 <mehtamanik96@gmail.com>

Manik2708 · 2025-03-20T09:01:24Z

internal/storage/v1/elasticsearch/spanstore/internal/dbmodel/from_domain_test.go

 	}
 }
+
+func TestNewSpanTags(t *testing.T) {


This test was previously present in writer_test.go, it was testing FromDomainEmbedProcess via reader, for example this is how it was testing in writer: test.writer.spanWriter.spanConverter.FromDomainEmbedProcess(s). If it's only aim is to test a method of FromDomain, then it should be present in from_domain not in writer

Manik2708 · 2025-03-21T13:48:16Z

@yurishkuro Please can you review this PR! As this is a blocker for the next PR.

internal/storage/v1/elasticsearch/spanstore/internal/dbmodel/from_domain_test.go

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

yurishkuro · 2025-03-21T14:36:22Z

As this is a blocker for the next PR.

why is it a blocker? Your next PR is introducing v2 writer, correct? It's a minor adjustment to that PR where the core writer is located.

Manik2708 · 2025-03-21T16:37:42Z

As this is a blocker for the next PR.

why is it a blocker? Your next PR is introducing v2 writer, correct? It's a minor adjustment to that PR where the core writer is located.

No, my next PR will be moving dbmodel to shared package and CoreSpanReader/Writer to v2, so implementing this is necessary.

yurishkuro · 2025-03-21T17:12:43Z

No, my next PR will be moving dbmodel to shared package and CoreSpanReader/Writer to v2, so implementing this is necessary.

I think you're taking this too literally. You can start work on implementing v2 writer, it doesn't matter where the core writer is currently located. We can always move it later.

Signed-off-by: Manik2708 <mehtamanik96@gmail.com>

yurishkuro · 2025-03-21T17:29:44Z

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

+	converter := dbmodel.NewFromDomain(true, []string{}, "-")
+	writerV1 := &SpanWriterV1{spanWriter: coreWriter, spanConverter: converter}


why are you not using NewSpanWriterV1()?

It is more similar to

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

Line 24 in a0d2100

spanReader: spanReader,

We can't use the mocks by using the NewSpanWriterV1() (as it takes SpanWriterParams)

We can't use the mocks by using the NewSpanWriterV1() (as it takes SpanWriterParams)

why can't we use mocks with SpanWriterParams?

Because SpanWriterParams initializes span writer by creating CoreSpanWriter in this way:

func NewSpanWriterV1(p SpanWriterParams) *SpanWriterV1 { return &SpanWriterV1{ spanWriter: NewSpanWriter(p), spanConverter: dbmodel.NewFromDomain(p.AllTagsAsFields, p.TagKeysAsFields, p.TagDotReplacement), } }

Further NewSpanWriter(p) initalizes this way:

func NewSpanWriter(p SpanWriterParams) *SpanWriter { serviceCacheTTL := p.ServiceCacheTTL if p.ServiceCacheTTL == 0 { serviceCacheTTL = serviceCacheTTLDefault } writeAliasSuffix := "" if p.UseReadWriteAliases { if p.WriteAliasSuffix != "" { writeAliasSuffix = p.WriteAliasSuffix } else { writeAliasSuffix = "write" } } serviceOperationStorage := NewServiceOperationStorage(p.Client, p.Logger, serviceCacheTTL) return &SpanWriter{ client: p.Client, logger: p.Logger, serviceWriter: serviceOperationStorage.Write, spanServiceIndex: getSpanAndServiceIndexFn(p, writeAliasSuffix), } }

But in tests we need mocks.CoreSpanWriter, therefore we can't use these methods to use mocks.

but you can use mock Client. You need at least one unit test that invokes the constructor.

) ## Which problem is this PR solving? Fixes a part of: jaegertracing#6458 ## Description of the changes - We have to prepeare the writer to move to `v2` and for that we need to separate `SpanWriter` ## 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 <mehtamanik96@gmail.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

) ## Which problem is this PR solving? Fixes a part of: jaegertracing#6458 ## Description of the changes - We have to prepeare the writer to move to `v2` and for that we need to separate `SpanWriter` ## 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 <mehtamanik96@gmail.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: amol-verma-allen <amol.verma@allen.in>

## Which problem is this PR solving? - Resolves #6891 ## Description of the changes - Reintroduced metrics which were lost during #6883 ## How was this change tested? - Ran writer 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: anmol7344 <anmol7344@gmail.com> Signed-off-by: Anmol <166167480+AnmolxSingh@users.noreply.github.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <github@ysh.us>

## Which problem is this PR solving? - Resolves jaegertracing#6891 ## Description of the changes - Reintroduced metrics which were lost during jaegertracing#6883 ## How was this change tested? - Ran writer 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: anmol7344 <anmol7344@gmail.com> Signed-off-by: Anmol <166167480+AnmolxSingh@users.noreply.github.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <github@ysh.us>

[ES] Separated the CoreSpanWriter from SpanWriter

63e9b58

Signed-off-by: Manik2708 <mehtamanik96@gmail.com>

Manik2708 requested a review from a team as a code owner March 19, 2025 19:45

Manik2708 requested a review from jkowall March 19, 2025 19:45

Merge branch 'main' into move

d17ae74

dosubot bot added storage/elasticsearch v2 labels Mar 19, 2025

Manik2708 commented Mar 19, 2025

View reviewed changes

Manik2708 changed the title ~~[ES] Separated the CoreSpanWriter from SpanWriter~~ [ES] Separate the CoreSpanWriter from SpanWriter Mar 19, 2025

code-cov

bc4692f

Signed-off-by: Manik2708 <mehtamanik96@gmail.com>

yurishkuro reviewed Mar 20, 2025

View reviewed changes

Manik2708 added 2 commits March 20, 2025 14:25

cleanup

22c291c

Signed-off-by: Manik2708 <mehtamanik96@gmail.com>

Merge branch 'main' into move

7be46ff

Manik2708 requested a review from yurishkuro March 20, 2025 08:55

Manik2708 commented Mar 20, 2025

View reviewed changes

yurishkuro added the changelog:refactoring Internal code refactoring without functional changes label Mar 21, 2025

yurishkuro reviewed Mar 21, 2025

View reviewed changes

internal/storage/v1/elasticsearch/spanstore/internal/dbmodel/from_domain_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 21, 2025

View reviewed changes

internal/storage/v1/elasticsearch/spanstore/writerv1.go Outdated Show resolved Hide resolved

Manik2708 added 2 commits March 21, 2025 22:51

cleanup

c852d17

Signed-off-by: Manik2708 <mehtamanik96@gmail.com>

Merge branch 'main' into move

bb21f65

Manik2708 requested a review from yurishkuro March 21, 2025 17:23

yurishkuro reviewed Mar 21, 2025

View reviewed changes

Merge branch 'main' into move

40360b2

yurishkuro approved these changes Mar 22, 2025

View reviewed changes

yurishkuro enabled auto-merge March 22, 2025 17:17

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

auto-merge was automatically disabled March 22, 2025 17:33
Pull Request is not mergeable

Merged via the queue into jaegertracing:main with commit 23cf383 Mar 22, 2025
56 checks passed

AnmolxSingh mentioned this pull request Apr 9, 2025

[Fix] Restore ES metrics #7006

Merged

4 tasks

		converter := dbmodel.NewFromDomain(true, []string{}, "-")
		writerV1 := &SpanWriterV1{spanWriter: coreWriter, spanConverter: converter}

Conversation

Manik2708 commented Mar 19, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

Manik2708 Mar 19, 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!

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

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Manik2708 Mar 20, 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!

Manik2708 commented Mar 21, 2025

Uh oh!

Uh oh!

Uh oh!

yurishkuro commented Mar 21, 2025

Uh oh!

Manik2708 commented Mar 21, 2025

Uh oh!

yurishkuro commented Mar 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manik2708 Mar 21, 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!

Choose a reason for hiding this comment

Uh oh!

Manik2708 Mar 22, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Manik2708 Mar 19, 2025 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

Manik2708 Mar 20, 2025 •

edited

Loading

Manik2708 Mar 21, 2025 •

edited

Loading

Manik2708 Mar 22, 2025 •

edited

Loading