Fix propagation issues for context in the new obsreport usage #625

bogdandrutu · 2020-03-12T00:22:27Z

No description provided.

bogdandrutu

Almost every receiver had an issue with passing the created Span or the Tags to the next component :(

bogdandrutu · 2020-03-12T00:31:33Z

receiver/jaegerreceiver/trace_receiver.go

-	_, span := obsreport.StartTraceDataReceiveOp(
-		receiverCtx, jr.instanceName, collectorHTTPTransport)


Here it was a bug because the Span was not propagated to the consumer.

bogdandrutu · 2020-03-12T00:31:40Z

receiver/jaegerreceiver/trace_receiver.go

-	_, span := obsreport.StartTraceDataReceiveOp(
-		ctx, jtr.instanceName, collectorTChannelTransport)


Here it was a bug because the Span was not propagated to the consumer.

bogdandrutu · 2020-03-12T00:32:11Z

receiver/jaegerreceiver/trace_receiver.go

-	_, span := obsreport.StartTraceDataReceiveOp(
-		context.Background(), jr.instanceName, agentTransport)


Here it was a bug because the Span was not propagated to the consumer.

bogdandrutu · 2020-03-12T00:33:01Z

receiver/jaegerreceiver/trace_receiver.go

-	_, span := obsreport.StartTraceDataReceiveOp(
-		ctx, jr.instanceName, grpcTransport)


Here it was a bug because the Span was not propagated to the consumer.

bogdandrutu · 2020-03-12T00:33:27Z

receiver/opencensusreceiver/ocmetrics/opencensus.go

@@ -113,13 +114,17 @@ func (ocr *Receiver) processReceivedMetrics(longLivedRPCCtx context.Context, ni
 func (ocr *Receiver) sendToNextConsumer(longLivedRPCCtx context.Context, md consumerdata.MetricsData) {
 	// Do not use longLivedRPCCtx to start the span so this trace ends right at this
 	// function, and the span is not a child of any span from the stream context.
-	_, span := obsreport.StartMetricsReceiveOp(
+	tmpCtx := obsreport.StartMetricsReceiveOp(
 		context.Background(),
 		ocr.instanceName,
 		receiverTransport)


Here it was a bug because the Span was not propagated to the consumer.

bogdandrutu · 2020-03-12T00:35:38Z

receiver/opencensusreceiver/octrace/opencensus.go

@@ -160,7 +165,7 @@ func (ocr *Receiver) sendToNextConsumer(longLivedRPCCtx context.Context, traceda
 		err = ocr.nextConsumer.ConsumeTraceData(ctx, *tracedata)


Bug: We did not pass the tags from the longLivedRPCCtx to the consumer so metrics recorded in the producer/exporter will not have them.

codecov-io · 2020-03-12T00:39:14Z

Codecov Report

Merging #625 into master will decrease coverage by <.01%.
The diff coverage is 85.1%.

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
- Coverage   75.27%   75.26%   -0.01%     
==========================================
  Files         139      139              
  Lines        9674     9672       -2     
==========================================
- Hits         7282     7280       -2     
  Misses       2072     2072              
  Partials      320      320

Impacted Files	Coverage Δ
obsreport/obsreport.go	`66.66% <0%> (-1.2%)`	⬇️
receiver/zipkinreceiver/trace_receiver.go	`60.84% <0%> (ø)`	⬆️
...eceiver/opencensusreceiver/ocmetrics/opencensus.go	`84.61% <100%> (ø)`	⬆️
exporter/exporterhelper/tracehelper.go	`93.33% <100%> (ø)`	⬆️
obsreport/obsreport_exporter.go	`100% <100%> (ø)`	⬆️
exporter/exporterhelper/metricshelper.go	`95.34% <100%> (-0.11%)`	⬇️
receiver/jaegerreceiver/trace_receiver.go	`85.88% <100%> (-0.06%)`	⬇️
receiver/opencensusreceiver/octrace/opencensus.go	`75.43% <100%> (+0.43%)`	⬆️
obsreport/obsreport_receiver.go	`100% <100%> (ø)`	⬆️
...eceiver/prometheusreceiver/internal/transaction.go	`77.45% <66.66%> (+0.75%)`	⬆️

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 5334b3a...20a532d. Read the comment docs.

Signed-off-by: Bogdan Drutu <[email protected]>

tigrannajaryan

Will you have some time to do a quick presentation on how to do the context propagation correctly? I have no idea how this code works and I suspect there may be others in the team who may benefit from what you can tell.

pjanotti

Thanks for catching my bugs @bogdandrutu

pjanotti · 2020-03-12T16:50:25Z

obsreport/obsreport_receiver.go

@@ -75,7 +75,7 @@ func StartTraceDataReceiveOp(
 	operationCtx context.Context,
 	receiver string,
 	transport string,
-) (context.Context, *trace.Span) {


Ok, it makes sense: the few cases that one may want to add something to the span can be done by extracting it from context.

pjanotti · 2020-03-12T17:08:11Z

receiver/opencensusreceiver/ocmetrics/opencensus.go

+
+	// TODO: We should offer a version of StartTraceDataReceiveOp that starts the Span
+	// with as root, and links to the longLived context.
+	ctx := trace.NewContext(longLivedRPCCtx, trace.FromContext(tmpCtx))


Thanks, I wasn't aware that this is the correct way to propagate the tags from parent while still having a trace for the individual operation. I will work on getting the TODO done.

bogdandrutu requested review from flands, owais, pjanotti, rghetia, songy23 and tigrannajaryan as code owners March 12, 2020 00:22

bogdandrutu force-pushed the obsrec branch from 3dd360b to 20a532d Compare March 12, 2020 00:29

bogdandrutu commented Mar 12, 2020

View reviewed changes

Fix propagation issues for context in the new obsreport usage

eab5707

Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu force-pushed the obsrec branch from 20a532d to eab5707 Compare March 12, 2020 00:50

tigrannajaryan reviewed Mar 12, 2020

View reviewed changes

pjanotti approved these changes Mar 12, 2020

View reviewed changes

tigrannajaryan approved these changes Mar 12, 2020

View reviewed changes

tigrannajaryan merged commit 4f385cd into open-telemetry:master Mar 12, 2020

bogdandrutu deleted the obsrec branch August 5, 2020 18:49

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

translatesfx: translate SA filter negation (open-telemetry#625)

a83f4f9

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

move atomic_shared_ptr to sdk common namespace (open-telemetry#625)

1a3e6cc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix propagation issues for context in the new obsreport usage #625

Fix propagation issues for context in the new obsreport usage #625

bogdandrutu commented Mar 12, 2020

bogdandrutu left a comment

bogdandrutu Mar 12, 2020

bogdandrutu Mar 12, 2020

bogdandrutu Mar 12, 2020

bogdandrutu Mar 12, 2020

bogdandrutu Mar 12, 2020

bogdandrutu Mar 12, 2020

codecov-io commented Mar 12, 2020

tigrannajaryan left a comment

pjanotti left a comment

pjanotti Mar 12, 2020

pjanotti Mar 12, 2020

		_, span := obsreport.StartTraceDataReceiveOp(
		receiverCtx, jr.instanceName, collectorHTTPTransport)

		_, span := obsreport.StartTraceDataReceiveOp(
		ctx, jtr.instanceName, collectorTChannelTransport)

		_, span := obsreport.StartTraceDataReceiveOp(
		context.Background(), jr.instanceName, agentTransport)

		_, span := obsreport.StartTraceDataReceiveOp(
		ctx, jr.instanceName, grpcTransport)

		@@ -160,7 +165,7 @@ func (ocr *Receiver) sendToNextConsumer(longLivedRPCCtx context.Context, traceda
		err = ocr.nextConsumer.ConsumeTraceData(ctx, *tracedata)

Fix propagation issues for context in the new obsreport usage #625

Fix propagation issues for context in the new obsreport usage #625

Conversation

bogdandrutu commented Mar 12, 2020

bogdandrutu left a 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-io commented Mar 12, 2020

Codecov Report

tigrannajaryan left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment