StartSpan should not generate active span by default #994
StartSpan should not generate active span by default #994cijothomas merged 17 commits intoopen-telemetry:masterfrom rajkumar-rangaraj:rajrang/tracer_span
Conversation
| /// <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.
Lets modify the summary to say this will not set the started span as the active one.
There was a problem hiding this comment.
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.
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.
| } | ||
|
|
||
| /// <summary> | ||
| /// Starts an active span. |
There was a problem hiding this comment.
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.
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.
| [Fact] | ||
| public void Tracer_StartRootSpan_BadArgs_NullSpanName() | ||
| { | ||
| using var openTelemetry = Sdk.CreateTracerProvider( |
There was a problem hiding this comment.
name is required parameter. so may be we are better off throw invalidargexception?
There was a problem hiding this comment.
Should we change the implementation?
|
@rajkumar-rangaraj can you resolve conflict? |
Done. |
2 participants
Fixes #989 .
Changes
Please provide a brief description of the changes here. Update the
CHANGELOG.mdfor non-trivial changes.StartRootSpanandStartActiveSpantoTracerAPIOpenTracing Shimto use StartRootSpan and fixed TODOFor significant contributions please make sure you have completed the following items: