fix(http): do not set outgoing http span as active in the context #1479

[vmarchaud]

See #1479 for motivation

[codecov]

Codecov Report

Merging #1546 into master will decrease coverage by 0.01%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #1546      +/-   ##
==========================================
- Coverage   91.50%   91.48%   -0.02%     
==========================================
  Files         165      165              
  Lines        5036     5036              
  Branches     1025     1025              
==========================================
- Hits         4608     4607       -1     
- Misses        428      429       +1     

Impacted Files	Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts	97.26% <87.50%> (-0.55%)	⬇️

[dyladan]

Agree with @blumamir's suggested test, but this seems like a good idea to me.

[obecny]

@vmarchaud this PR duplicates similar stuff I fixed here -> #1541

[blumamir]

@obecny This PR only removes the withSpan from outgoing HTTP requests. Your PR Still has it. What am I missing?

[obecny]

@obecny This PR only removes the withSpan from outgoing HTTP requests. Your PR Still has it. What am I missing?

maybe I'm missing something but trying to analyze possible merge between 2 prs and wondering if it won't be either duplicated or the outcome will be different for both cases.

[vmarchaud]

@vmarchaud this PR duplicates similar stuff I fixed here -> #1541

You are right however that wasn't the goal of the PR in the first place. I think we could merge your PR first and then i will rebase my PR to remove the tracer.withSpan.

[obecny]

@vmarchaud this PR duplicates similar stuff I fixed here -> #1541

You are right however that wasn't the goal of the PR in the first place. I think we could merge your PR first and then i will rebase my PR to remove the tracer.withSpan.

make sense, thx :)

[vmarchaud]

I've rebased the PR, i believe this is a simpler fix than #1541 that does involves injecting anything in the context.

[vmarchaud]

cc @open-telemetry/javascript-approvers Would like more review to ship it in the next release please !

[dyladan]

Motivating example for those not sure.

Scenario: You want to make 2 requests. The second request should not be made until after the first request completes.

With fix:

const parent = tracer.startSpan("parent");

tracer.withSpan(parent, () => {
	console.log(tracer.getActiveSpan().name); // parent
	http.get(url, (res) => { // new span is created by the http plugin
		res.resume() // discard response data
		res.on('end', () => {
			// inside this callback, the http.GET span should NOT be active
			console.log(tracer.getActiveSpan().name); // parent - Success!
            
            // the span for this request is a sibling of the first request as expected
			http.get(url, (res) => {/* elided */}) 
		})
	});
});

without fix

const parent = tracer.startSpan("parent");

tracer.withSpan(parent, () => {
	console.log(tracer.getActiveSpan().name); // parent
	http.get(url, (res) => { // new span is created by the http plugin
		res.resume() // discard response data
		res.on('end', () => {
			// inside this callback, the http.GET span should NOT be active
			console.log(tracer.getActiveSpan().name); // GET - Oh no! D:  <-------- bug here
            
            // the span for this request is a child of the first request
            // even though it is not _caused_by_ the first request.
			http.get(url, (res) => {/* elided */}) 
		})
	});
});

[vmarchaud]

@mwear Would be really nice to review this one so we can ship it with 0.12 ? Thanks

[mwear]

@mwear Would be really nice to review this one so we can ship it with 0.12 ? Thanks

Sorry I missed the boat on this @vmarchaud.