Fix scope management in StartSpan and doFinish

[Darevski]

Problem:
Previously, when creating a chain of spans, events triggered with the sentry.CaptureMessage() could be associated with the wrong span due to using the incorrect span in the hub’s scope, which led to inaccurate trace data in Sentry.

Fix:
By introducing proper span scope management, each non-transaction span now pushes a scope when started and pops it when finished. This ensures that events are associated with the correct active span in the trace hierarchy. Transactions remain unchanged as the root elements.

Test Coverage:
The new TestSpanScopeManagement confirms that events captured after finishing a child span are correctly associated with that span, ensuring that the trace and span IDs are accurate.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.86%. Comparing base (a6acd05) to head (5407f87).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #886      +/-   ##
==========================================
+ Coverage   82.85%   82.86%   +0.01%     
==========================================
  Files          55       55              
  Lines        4619     4623       +4     
==========================================
+ Hits         3827     3831       +4     
  Misses        636      636              
  Partials      156      156              

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

[cleptric]

It seems very wasteful to create a new scope for each span. With the recent changes in 0.29.0 (/~https://github.com/getsentry/sentry-go/blob/master/tracing.go#L197-L198), this should already work, considering you're setting the correct hub on the context.

[Darevski]

@cleptric

It seems very wasteful to create a new scope for each span. With the recent changes in 0.29.0 (/~https://github.com/getsentry/sentry-go/blob/master/tracing.go#L197-L198), this should already work, considering you're setting the correct hub on the context.

With version 0.29.0, the following code:

	transaction := sentry.StartTransaction(ctx, "parent-operation")
	defer transaction.Finish()

	childSpan := sentry.StartSpan(transaction.Context(), "child-operation")
	defer childSpan.Finish()

	subChildSpan := sentry.StartSpan(childSpan.Context(), "sub_child-operation")
	subChildSpan.Finish()
	
	sentry.CaptureMessage("Test event")

The code will act like this: the “Test event” message will be assigned to the already finished ‘sub_child-operation’ span instead of the ‘child-operation’ span.
Is this the correct behaviour? I don’t think so.

The same thing happens with all methods that use hub.scope internally.

You could take the test that I prepared for that case from my PR if you need it.

But the biggest problems start when you try to create distributed tracing

Here is the working sample code example:
https://go.dev/play/p/OudKFJv1OL0

This is the trace view result of the code with my fix:

We have a fully functional informative view, with the messages correctly assigned to the required spans and transactions.

And here is the result of this code without my fix (0.29.0):

Chain connection are missing

We could try to look deeper into the requests that are being sent to Sentry:

Here is the request without the fix (removed unusable info):

We could see that when we sending the "child-transaction", the "child-span" is related to the "parent-span" (from the first service) instead of the "child-transaction."

Here is the request with the fix:

Here we can see that the "child-span" correctly references the "child-transaction" span.

In the case that we start using OpenTelemetry, we encounter far more problems, as a lot of information is placed inside the scope span, and the hub.Scope().SetSpan(&span) call simply overrides it (the child span overrides the parent scope without any restore).

Could you kindly clarify what exactly I should do in this situation without modifying the Sentry codebase? Are you suggesting I should create a new hub for every span, or is there a more practical solution I’m missing?

[shlyamster]

@cleptric I confirm the presence of the problem described here in version 0.29.0. In my traces the same way root context is lost because of which the span order in the transaction is not correctly organized.

Here is an example trace on version 0.29.0. Note the Parent Span ID field at the root transaction.

And here's the trace with the suggested fixes.

[Darevski]

@cleptric, the fix in #888 is only a partial solution, as it only resolves the trace context for the transaction.
What about capturing events like messages or exceptions, which I mentioned two messages earlier?

The main issue is that the scope retains the state after starting spans and holds onto that state indefinitely, until a new span is created. This scope is then applied to all subsequent events.

If there is a large trace with some information or exceptions, it’s very useful to see in which span that event was created. However, currently, it is connected to the most recently created span.

If you think that my fix is wasteful, could you please advise what could be done to resolve this?

Our company is really need this functionality of correct attaching events