-
Notifications
You must be signed in to change notification settings - Fork 757
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
StartSpan should not generate active span by default #994
StartSpan should not generate active span by default #994
Conversation
/// <param name="parentSpan">Parent for new span.</param> | ||
/// <param name="attributes">Initial attributes for the span.</param> | ||
/// <param name="links"> <see cref="Link"/> for the span.</param> | ||
/// <param name="startTime"> Start time for the span.</param> | ||
/// <returns>Span instance.</returns> | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets modify the summary to say this will not set the started span as the active one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the spec and other languages implementation, starting a span does not make it active, should we call it out explicitly here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is important for .NET, as the .NET Activity API's StartActivity is making it active (this is violation of spec). The equivalent SHIM api should be StartActiveSpan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
|
||
/// <summary> | ||
/// Starts an active span. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starts a span and make it the current active span.
Codecov Report
@@ Coverage Diff @@
## master #994 +/- ##
==========================================
+ Coverage 68.56% 68.84% +0.27%
==========================================
Files 220 220
Lines 6003 5999 -4
Branches 984 984
==========================================
+ Hits 4116 4130 +14
+ Misses 1611 1598 -13
+ Partials 276 271 -5
|
/// <returns>Span instance.</returns> | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public TelemetrySpan StartSpan(string name, SpanKind kind = SpanKind.Internal) | ||
public TelemetrySpan StartRootSpan(string name, SpanKind kind = SpanKind.Internal, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add Tracer unit tests to validate all the scenarios? RootSpan, Start, StartActive? We had tests before, but I got them removed when shifting to Activity. We can now add some tests.
…garaj/opentelemetry-dotnet into rajrang/tracer_span
[Fact] | ||
public void Tracer_StartRootSpan_BadArgs_NullSpanName() | ||
{ | ||
using var openTelemetry = Sdk.CreateTracerProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name is required parameter. so may be we are better off throw invalidargexception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change the implementation?
@rajkumar-rangaraj can you resolve conflict? |
Done. |
Fixes #989 .
Changes
Please provide a brief description of the changes here. Update the
CHANGELOG.md
for non-trivial changes.StartRootSpan
andStartActiveSpan
toTracer
APIOpenTracing Shim
to use StartRootSpan and fixed TODOFor significant contributions please make sure you have completed the following items: