apm-agent-dotnet icon indicating copy to clipboard operation
apm-agent-dotnet copied to clipboard

Never return `null` from `Tracer.CurrentTransaction` and `Tracer.CurrentSpan`

Open gregkalapos opened this issue 3 years ago • 3 comments

Follow up from: https://github.com/elastic/apm-agent-dotnet/pull/997#discussion_r520120506

Now we have NoopTransaction and NoopSpan in place - let's always return those instead of null if there is no active transaction/span.

Advantage:

You can always write this, and user code will always be executed:

agent.Tracer.CurrentTransaction.CaptureSpan("foo", "bar", () =>
{
	//user code to run
});

No need for null-checks and agent.Tracer.CurrentTransaction will never be null.

gregkalapos avatar Nov 10 '20 14:11 gregkalapos

Agreed with CurrentTransaction but have concerns for CurrentSpan. As far as I know, it's better to start a new span on the current span if it's presented to save call hierarchy. For this reason, some instrumentation places are looking into CurrentSpan and then fallback to CurrentTransaction.

https://github.com/elastic/apm-agent-dotnet/blob/b609a01ea1b45f8a019e9bf812a2859c869a1d7b/src/Elastic.Apm/Model/ExecutionSegmentCommon.cs#L242-L243 https://github.com/elastic/apm-agent-dotnet/blob/2c433d31c45444e026081721e9c4190dadc383b8/docs/public-api.asciidoc#manually-propagating-distributed-tracing-context

As far as I understand your suggestion correctly, CurrentSpan will return NoopSpan instead of null which will lead to instrumentation issues: no any way to set labels, no any ability to capture Span as it returns NoopSpan instance

vhatsura avatar Nov 10 '20 14:11 vhatsura

@gregkalapos how are you checking for null?

No need for null-checks and agent.Tracer.CurrentTransaction will never be null.

I'm facing the same issue where I know there is a transaction in production but not in my unit tests. I can't see a way to null check without repeating all my code in 2 branches.

christamlyn-bridge avatar May 24 '21 15:05 christamlyn-bridge

For this reason, some instrumentation places are looking into CurrentSpan and then fallback to CurrentTransaction.

@vhatsura If CurrentTransaction falls back to NoopTransaction, CurrentSpan will in these cases work as expected. Also, is it ideal that the instrumentation does not always fall back to CurrentTransaction?

While I appreciate the project a great deal, retrofitting APM into existing code using the Capture* comes with hardship in making tests pass. I have found no good way to instrument test code so that the action is run when Agent.Tracer.Current* are null.

ehvattum avatar Jun 15 '22 10:06 ehvattum