Replace es-spanstore tracing instrumentation with OpenTelemetry #4596

afzal442 · 2023-07-23T09:32:28Z

Which problem is this PR solving?

Part of Upgrade internal use of tracing to OpenTelemetry #3381
This PR adds OTEL Provider to es-spanstore component

Short description of the changes

Replaces OT tracer with OTEL tracer to support jtracer pkg

Signed-off-by: Afzal Ansari <[email protected]>

codecov · 2023-07-23T09:38:11Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (2c1bf07) 97.08% compared to head (a98a0ed) 97.07%.
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4596      +/-   ##
==========================================
- Coverage   97.08%   97.07%   -0.01%     
==========================================
  Files         301      301              
  Lines       17857    17870      +13     
==========================================
+ Hits        17336    17347      +11     
- Misses        418      419       +1     
- Partials      103      104       +1

Flag	Coverage Δ
unittests	`97.07% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
plugin/storage/es/factory.go	`97.65% <100.00%> (+0.03%)`	⬆️
plugin/storage/es/spanstore/reader.go	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader.go

plugin/storage/es/spanstore/reader_test.go

plugin/storage/es/spanstore/reader.go

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader_test.go

plugin/storage/es/spanstore/reader.go

plugin/storage/es/factory.go

plugin/storage/integration/elasticsearch_test.go

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader.go

plugin/storage/es/factory.go

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader_test.go

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader.go

plugin/storage/es/spanstore/reader_test.go

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader.go

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader_test.go

Signed-off-by: Afzal Ansari <[email protected]>

plugin/storage/es/spanstore/reader_test.go

Signed-off-by: Afzal Ansari <[email protected]>

yurishkuro · 2023-07-30T16:55:07Z

plugin/storage/es/spanstore/reader.go

+	if p.Tracer == nil {
+		p.Tracer = otel.Tracer("eSpanstore.SpanReader")
+	}


Suggested change

if p.Tracer == nil {

p.Tracer = otel.Tracer("eSpanstore.SpanReader")

}

plugin/storage/es/spanstore/reader.go

yurishkuro · 2023-07-30T16:56:49Z

plugin/storage/es/spanstore/reader_test.go

+		client:       client,
+		logger:       logger,
+		logBuffer:    logBuffer,
+		exporter:     exp,


Suggested change

exporter: exp,

traceBuffer: exp,

yurishkuro · 2023-07-30T17:03:46Z

plugin/storage/es/spanstore/reader_test.go

@@ -307,6 +327,8 @@ func TestSpanReader_GetTrace(t *testing.T) {
 			}, nil)

 		trace, err := r.reader.GetTrace(context.Background(), model.NewTraceID(0, 1))
+		require.NotEmpty(t, r.exporter.GetSpans(), "Spans recorded")
+		require.NoError(t, r.tracerCloser())


one of the nice things about doing tests in a functional style via withSpanReader is that withSpanReader can do both initialization and tear-down, since it's frame is lower in the stack than the actual test function and defers inside withSpanReader always get executed even if the test function panics.

TestSomething() withSpanReader(testFn) d = initialize() defer d.teardown() testFn(d)

So I don't think we even need to expose traceCloser in the struct, we can just defer-call it inside withSpanReader.

This is very different from helper functions that simply return a tracer. They have no room to defer-schedule shutdown, and you have to do it manually in the caller.

That's nit. Cherry picked. 😄

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

yurishkuro · 2023-07-30T18:41:13Z

plugin/storage/es/spanstore/reader_test.go

@@ -126,6 +124,7 @@ func withSpanReader(fn func(r *spanReaderTest)) {
 			MaxDocCount:       defaultMaxDocCount,
 		}),
 	}
+	defer closer()


we talked about where defers should be placed

sorry Yuri! I did it inside withSpanReader function call. but How can I defer it? I think I have to modify the closer to use func() and fmt.Errorf. Or if I mistaken here, will I have to place it inside Test Function(fn)?

you create the object in L110, so defer closer() should be L111. Always keep defer next to creation.

I did that. Just to re-commit.

yurishkuro · 2023-07-30T18:44:58Z

plugin/storage/es/spanstore/reader_test.go

 		r := NewSpanReader(testCase.params)

 		actualSpan := r.timeRangeIndices(r.spanIndexPrefix, r.spanIndexDateLayout, date, date, -1*time.Hour)
 		actualService := r.timeRangeIndices(r.serviceIndexPrefix, r.serviceIndexDateLayout, date, date, -24*time.Hour)
 		assert.Equal(t, testCase.indices, append(actualSpan, actualService...))
 	}
+	require.NoError(t, closer())


defer after creation

yurishkuro · 2023-07-30T18:47:26Z

plugin/storage/integration/elasticsearch_test.go

@@ -160,6 +177,10 @@ func (s *ESStorageIntegration) initSpanstore(allTagsAsFields, archive bool) erro
 		MaxDocCount:     defaultMaxDocCount,
 	})

+	if closer != nil {
+		return err


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

yurishkuro · 2023-07-30T19:50:37Z

plugin/storage/es/spanstore/reader_test.go

 			MaxSpanAge:          0,
 			IndexPrefix:         "",
 			TagDotReplacement:   "@",
 			Archive:             true,
 			UseReadWriteAliases: readAlias,
 		}),
 	}
+	defer closer()


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

yurishkuro

lgtm

yurishkuro · 2023-07-30T21:04:37Z

minor clean-up #4605

afzal442 added 2 commits July 23, 2023 09:14

adds otel tracer to spanReader

db42f17

Signed-off-by: Afzal Ansari <[email protected]>

updates jtracer API call

a9b5adb

Signed-off-by: Afzal Ansari <[email protected]>

afzal442 requested a review from a team as a code owner July 23, 2023 09:32

afzal442 requested a review from vprithvi July 23, 2023 09:32

rmvs defer for err

e61c5c4

Signed-off-by: Afzal Ansari <[email protected]>

afzal442 commented Jul 23, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Outdated Show resolved Hide resolved

afzal442 commented Jul 23, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jul 23, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Outdated Show resolved Hide resolved

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

plugin/storage/es/spanstore/reader.go Outdated Show resolved Hide resolved

adds otel tracer as a global

8577650

Signed-off-by: Afzal Ansari <[email protected]>

afzal442 commented Jul 24, 2023

View reviewed changes

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jul 24, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jul 24, 2023

View reviewed changes

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jul 24, 2023

View reviewed changes

plugin/storage/integration/elasticsearch_test.go Outdated Show resolved Hide resolved

adds tracer to factory to init

f0fbead

Signed-off-by: Afzal Ansari <[email protected]>

yurishkuro reviewed Jul 25, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Outdated Show resolved Hide resolved

afzal442 commented Jul 25, 2023

View reviewed changes

plugin/storage/es/factory.go Show resolved Hide resolved

sets otel to main

bdc5871

Signed-off-by: Afzal Ansari <[email protected]>

afzal442 changed the title ~~[WIP] Add OTEL tracer to spanstore component~~ [WIP] Add OTEL tracer to es-spanstore component Jul 26, 2023

adds test tracer to spanreader_test

33ea64e

Signed-off-by: Afzal Ansari <[email protected]>

yurishkuro reviewed Jul 27, 2023

View reviewed changes

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

adds tp to readertest

11840a0

Signed-off-by: Afzal Ansari <[email protected]>

afzal442 commented Jul 27, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Show resolved Hide resolved

yurishkuro reviewed Jul 27, 2023

View reviewed changes

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

makes closer as err

e1d5515

Signed-off-by: Afzal Ansari <[email protected]>

afzal442 commented Jul 28, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Show resolved Hide resolved

afzal442 added 2 commits July 28, 2023 18:08

adds otel tracer to reader

03ee675

Signed-off-by: Afzal Ansari <[email protected]>

replace assert with require

52d5069

Signed-off-by: Afzal Ansari <[email protected]>

afzal442 commented Jul 28, 2023

View reviewed changes

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

updates closer() call

8def751

Signed-off-by: Afzal Ansari <[email protected]>

yurishkuro reviewed Jul 28, 2023

View reviewed changes

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jul 29, 2023

View reviewed changes

plugin/storage/es/spanstore/reader_test.go Outdated Show resolved Hide resolved

afzalbin64 force-pushed the replace-with-otel-spanstore branch from 15a8ccd to 48f38ad Compare July 29, 2023 19:29

adds type closer

0d078fb

Signed-off-by: Afzal Ansari <[email protected]>

afzalbin64 force-pushed the replace-with-otel-spanstore branch from 48f38ad to 0d078fb Compare July 30, 2023 10:27

yurishkuro reviewed Jul 30, 2023

View reviewed changes

plugin/storage/es/spanstore/reader.go Show resolved Hide resolved

yurishkuro reviewed Jul 30, 2023

View reviewed changes

rmvs unwanted object init

f3e92fa

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

afzalbin64 force-pushed the replace-with-otel-spanstore branch from 0eab75b to f3e92fa Compare July 30, 2023 18:24

yurishkuro reviewed Jul 30, 2023

View reviewed changes

defers the call

a037b42

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

yurishkuro reviewed Jul 30, 2023

View reviewed changes

sets span status

a98a0ed

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

yurishkuro approved these changes Jul 30, 2023

View reviewed changes

yurishkuro changed the title ~~[WIP] Add OTEL tracer to es-spanstore component~~ Replace es-spanstore tracing instrumentation with OpenTelemetry Jul 30, 2023

yurishkuro merged commit de3cf99 into jaegertracing:main Jul 30, 2023
31 checks passed

afzalbin64 deleted the replace-with-otel-spanstore branch July 31, 2023 05:40

afzalbin64 restored the replace-with-otel-spanstore branch August 14, 2023 07:20

afzal442 deleted the replace-with-otel-spanstore branch August 14, 2023 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace es-spanstore tracing instrumentation with OpenTelemetry #4596

Replace es-spanstore tracing instrumentation with OpenTelemetry #4596

afzal442 commented Jul 23, 2023 •

edited

Loading

codecov bot commented Jul 23, 2023 •

edited

Loading

yurishkuro Jul 30, 2023

yurishkuro Jul 30, 2023

yurishkuro Jul 30, 2023 •

edited

Loading

afzalbin64 Jul 30, 2023

yurishkuro Jul 30, 2023

afzalbin64 Jul 30, 2023 •

edited

Loading

yurishkuro Jul 30, 2023

afzalbin64 Jul 30, 2023

yurishkuro Jul 30, 2023

yurishkuro Jul 30, 2023

yurishkuro Jul 30, 2023

yurishkuro Jul 30, 2023

yurishkuro left a comment

yurishkuro commented Jul 30, 2023

	if p.Tracer == nil {
	p.Tracer = otel.Tracer("eSpanstore.SpanReader")
	}

Replace es-spanstore tracing instrumentation with OpenTelemetry #4596

Replace es-spanstore tracing instrumentation with OpenTelemetry #4596

Conversation

afzal442 commented Jul 23, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jul 23, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afzalbin64 Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Jul 30, 2023

afzal442 commented Jul 23, 2023 •

edited

Loading

codecov bot commented Jul 23, 2023 •

edited

Loading

yurishkuro Jul 30, 2023 •

edited

Loading

afzalbin64 Jul 30, 2023 •

edited

Loading