apm-agent-dotnet
apm-agent-dotnet copied to clipboard
Never return `null` from `Tracer.CurrentTransaction` and `Tracer.CurrentSpan`
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
.
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
@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.
For this reason, some instrumentation places are looking into
CurrentSpan
and then fallback toCurrentTransaction
.
@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.