Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing test for StartRootSpan #2878

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Feb 9, 2022

Relates to #2803

Just adding a test for now for discussion and to determine whether there's something we can do at the moment.

It appears the StartActivity method starts a new span parented from Activity.Current even when you pass in a new ParentContext. I imagine this might be by design since Activity.Current is tracked via AsyncLocal and maybe StartActivity doesn't want to clobber that state.

public TelemetrySpan StartRootSpan(string name, SpanKind kind = SpanKind.Internal, SpanAttributes initialAttributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
{
return this.StartSpanHelper(false, name, kind, default, initialAttributes, links, startTime);
}

private TelemetrySpan StartSpanHelper(bool isActiveSpan, string name, SpanKind kind, in SpanContext parentContext = default, SpanAttributes initialAttributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
{
if (!this.ActivitySource.HasListeners())
{
return TelemetrySpan.NoopInstance;
}
var activityKind = ConvertToActivityKind(kind);
var activityLinks = links?.Select(l => l.ActivityLink);
Activity previousActivity = null;
if (!isActiveSpan)
{
previousActivity = Activity.Current;
}
var activity = this.ActivitySource.StartActivity(name, activityKind, parentContext.ActivityContext, initialAttributes?.Attributes ?? null, activityLinks, startTime);
if (activity == null)
{
return TelemetrySpan.NoopInstance;
}
if (!isActiveSpan)
{
Activity.Current = previousActivity;
}
return new TelemetrySpan(activity);
}

In a scenario where a thread hop has occurred - as in this test - maybe it would be more intuitive to wipe out Activity.Current... though, not sure there's a way for us to detect we're on a new thread.


Confirmed that StartActivity uses Activity.Current when the parent's ActivityContext == default

https://github.com/dotnet/runtime/blob/3ae87395f638a533f37b8e3385f6d3f199a72f4f/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs#L252-L253

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #2878 (8f75109) into main (7ce2e4c) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2878      +/-   ##
==========================================
+ Coverage   84.22%   84.23%   +0.01%     
==========================================
  Files         250      250              
  Lines        8883     8883              
==========================================
+ Hits         7482     7483       +1     
+ Misses       1401     1400       -1     
Impacted Files Coverage Δ
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️

test/OpenTelemetry.Tests/Trace/TracerTest.cs Outdated Show resolved Hide resolved
@cijothomas cijothomas merged commit b1d57e9 into open-telemetry:main Feb 9, 2022
@alanwest alanwest deleted the alanwest/add-startrootspan-failing-test branch February 9, 2022 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants