Add context.Context to WriteSpan #2436

yurishkuro · 2020-08-31T00:26:24Z

No description provided.

yurishkuro · 2020-08-31T00:30:06Z

cmd/collector/app/span_processor.go

@@ -140,7 +141,7 @@ func (sp *spanProcessor) saveSpan(span *model.Span) {
 	}

 	startTime := time.Now()
-	if err := sp.spanWriter.WriteSpan(span); err != nil {
+	if err := sp.spanWriter.WriteSpan(context.TODO(), span); err != nil {


Using TODO here because there is a design issue with spanProcessor, which should be managing the context earlier

Yep, for complete context propagation context needs to be added to the processor.SpanProcessor interface, we can do that in a follow up PR.

yurishkuro · 2020-08-31T00:31:00Z

cmd/ingester/app/processor/span_processor.go

 	if err != nil {
 		return fmt.Errorf("cannot unmarshall byte array into span: %w", err)
 	}
-	return s.writer.WriteSpan(mSpan)
+	return s.writer.WriteSpan(context.TODO(), span)


Using TODO here because KafkaSpanProcessor should be managing the context earlier.

yurishkuro · 2020-08-31T00:31:42Z

cmd/opentelemetry/app/receiver/kafkareceiver/kafka_receiver.go

 	batch := model.Batch{
 		Spans:   []*model.Span{span},
 		Process: span.Process,
 	}
 	traces := jaegertranslator.ProtoBatchToInternalTraces(batch)
+	// TODO what does w.ctx represent?


@pavolloffay can we get rid of w.ctx?

This is because of the metrics, check the line 38, Since the context is now propagated we should modify it here.

If I use the ctx arg in L93, I don't see where else w.ctx would be used. Should w.ctx be removed completely then? Or do we need to enrich ctx here similar to L38 and L40?

yes the w.ctx can be removed and instead the context can be modified in the write method.

ok, I made the changes, PTAL. But I think this needs to be revisited because it looks like a significant performance hit due to multiple memory allocations just to add some static labels. There should be a cheaper way of doing it, e.g. attaching a mutable structure to the Context at the start.

Anyways this kafka receiver will go away and we will use the one from OTEL upsteream.

yurishkuro · 2020-08-31T01:10:35Z

storage/spanstore/interface.go

-type Writer interface {
-	WriteSpan(span *model.Span) error
-}
-
 var (


constants must come before types

codecov · 2020-08-31T01:41:30Z

Codecov Report

Merging #2436 into master will decrease coverage by 0.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #2436      +/-   ##
==========================================
- Coverage   95.59%   95.58%   -0.02%     
==========================================
  Files         208      208              
  Lines       10681    10679       -2     
==========================================
- Hits        10211    10207       -4     
- Misses        399      400       +1     
- Partials       71       72       +1

Impacted Files	Coverage Δ
plugin/storage/integration/integration.go	`80.38% <0.00%> (ø)`
cmd/collector/app/span_processor.go	`100.00% <100.00%> (ø)`
cmd/ingester/app/processor/span_processor.go	`100.00% <100.00%> (ø)`
cmd/query/app/querysvc/query_service.go	`100.00% <100.00%> (ø)`
plugin/storage/badger/spanstore/writer.go	`97.53% <100.00%> (ø)`
plugin/storage/cassandra/spanstore/writer.go	`100.00% <100.00%> (ø)`
plugin/storage/es/spanstore/writer.go	`100.00% <100.00%> (ø)`
plugin/storage/grpc/shared/grpc_client.go	`85.96% <100.00%> (ø)`
plugin/storage/grpc/shared/grpc_server.go	`76.47% <100.00%> (ø)`
plugin/storage/kafka/writer.go	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c1deaf...af279ff. Read the comment docs.

pavolloffay

The context in the OTEL kafka has to be modified.

pavolloffay · 2020-08-31T06:55:26Z

cmd/opentelemetry/app/receiver/kafkareceiver/kafka_receiver.go

 	batch := model.Batch{
 		Spans:   []*model.Span{span},
 		Process: span.Process,
 	}
 	traces := jaegertranslator.ProtoBatchToInternalTraces(batch)
+	// TODO what does w.ctx represent?


This is because of the metrics, check the line 38, Since the context is now propagated we should modify it here.

annanay25 · 2020-09-01T07:08:46Z

cmd/collector/app/span_processor.go

@@ -140,7 +141,7 @@ func (sp *spanProcessor) saveSpan(span *model.Span) {
 	}

 	startTime := time.Now()
-	if err := sp.spanWriter.WriteSpan(span); err != nil {
+	if err := sp.spanWriter.WriteSpan(context.TODO(), span); err != nil {


Yep, for complete context propagation context needs to be added to the processor.SpanProcessor interface, we can do that in a follow up PR.

plugin/storage/es/spanstore/writer.go

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

* Add context.Context to WriteSpan Signed-off-by: Yuri Shkuro <[email protected]> * fmt Signed-off-by: Yuri Shkuro <[email protected]> * Regen mocks and fix tests Signed-off-by: Yuri Shkuro <[email protected]> * Remove context field Signed-off-by: Yuri Shkuro <[email protected]> * add TODOs Signed-off-by: Yuri Shkuro <[email protected]> * remove Context field Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: albertteoh <[email protected]>

yurishkuro requested a review from a team as a code owner August 31, 2020 00:26

yurishkuro requested a review from vprithvi August 31, 2020 00:26

yurishkuro commented Aug 31, 2020

View reviewed changes

yurishkuro force-pushed the writer-context branch from d650b81 to 0b05d47 Compare August 31, 2020 01:07

yurishkuro commented Aug 31, 2020

View reviewed changes

yurishkuro mentioned this pull request Aug 31, 2020

Grpc plugin archive storage support #2317

Merged

pavolloffay reviewed Aug 31, 2020

View reviewed changes

annanay25 reviewed Sep 1, 2020

View reviewed changes

yurishkuro added 5 commits September 1, 2020 15:41

Add context.Context to WriteSpan

1e95955

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

fmt

328813d

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

Regen mocks and fix tests

a63fd88

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

Remove context field

b64ddea

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

add TODOs

ad4419e

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

yurishkuro force-pushed the writer-context branch from 0b05d47 to ad4419e Compare September 1, 2020 19:45

remove Context field

af279ff

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

pavolloffay approved these changes Sep 2, 2020

View reviewed changes

pavolloffay merged commit 8630a31 into jaegertracing:master Sep 2, 2020

yurishkuro deleted the writer-context branch September 2, 2020 14:10

jpkrohling mentioned this pull request Sep 10, 2020

Add context to spanstore.Writer interface #2006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context.Context to WriteSpan #2436

Add context.Context to WriteSpan #2436

yurishkuro commented Aug 31, 2020

yurishkuro Aug 31, 2020

annanay25 Sep 1, 2020

yurishkuro Aug 31, 2020

yurishkuro Aug 31, 2020

pavolloffay Aug 31, 2020

yurishkuro Aug 31, 2020

pavolloffay Sep 1, 2020

yurishkuro Sep 1, 2020

pavolloffay Sep 2, 2020

yurishkuro Aug 31, 2020

codecov bot commented Aug 31, 2020 •

edited

Loading

pavolloffay left a comment

pavolloffay Aug 31, 2020

annanay25 Sep 1, 2020

Add context.Context to WriteSpan #2436

Add context.Context to WriteSpan #2436

Conversation

yurishkuro commented Aug 31, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2020 • edited Loading

Codecov Report

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2020 •

edited

Loading