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

Discussion: Support .NET Activities #128

Closed

Conversation

lmolkova
Copy link

@lmolkova lmolkova commented Jul 2, 2019

[Important] You don't really need to review 83 files - let's discuss it first! PR is only proof of concept.

.NET defines Activity API specifically for distributed tracing.

  • Activity represents Span and SpanContext together. It flows with async calls.
  • DiagnosticSource is somewhat similar to tracer

.NET libraries such as ASP.NET Core, ASP.NET, HTTP Client, SQL client (planned), EventHubs SDK, ServiceBus SDK, and in future all Azure client SDKs will be instrumented with Activities natively.

This is the best choice for them as it allows them to implement tracing without taking a dependency on anything else except .NET BCL.

OpenTelemtery must nicely work with it to leverage it and avoid extra perf costs.

This PR is for discussion on how to achieve it (and it's not easy). It is not full implementation and not even final - internal APIs probably need to change more. Please review public API area changes first and here they are.

Things that change drastically: PLEASE REVIEW THIS!

  • TraceId and SpanId are implemented in .NET. They removed from OpenTelemetry as well as random generators.
  • Each span gets an Activity property - you cannot start span without starting Activity
  • CurrentSpanUtils work through Activity - they attach Span to Activity - no more async local for Spans.
  • SpanBuilder gets new overloads
    • to start span from current Activity
    • to start a span as a child of current Activity.
  • Links API added to create link from Activity (you can extract Activity from queue messages)
  • Even though users may decide not to start scoped span (not to call WithSpan on tracer), Activity will be there (it always flows - you can explicitly prevent it only). In this case, if users keep using Spans without scopes and will propagate SpanContext explicitly - everything will still be working for them, they will simply ignore Activites.

So how it is going to be used
Example 1:

  • ASP.NET Core start a new Activity (it checks headers)
  • OpenTelemetry listens to ASP.NET Core events and creates a span from current Activity tracer.SpanBuilderFromActivity(path, SpanKind.Server, Activity.Current) and put it on the context
  • when user wants to create child span, they just call tracer.SpanBuilder(...).StartSpan() and by default, if becomes a child of current Span and current Activity

Example 2:

  • ServiceBus exposes method on the message to ExportActivity
  • Users call it and pass to tracer.SpanBuilderWithParentActivity(path, SpanKind.Server, message.ExportActivity())`

@SergeyKanzhelev
Copy link
Member

Things that change drastically: PLEASE REVIEW THIS!

  • TraceId and SpanId are implemented in .NET. They removed from OpenTelemetry as well as random generators.

Great!

  • Each span gets an Activity property - you cannot start span without starting Activity

You can still create a Span without setting Activity.Current, correct?

  • CurrentSpanUtils work through Activity - they attach Span to Activity - no more async local for Spans.
  • SpanBuilder gets new overloads
    • to start span from current Activity
    • to start a span as a child of current Activity.

So there may be a case when Activity without associated Span breaks parent/child relationship when Span is started? Should Spans be created for all Activities in the hierarchy?

  • Links API added to create link from Activity (you can extract Activity from queue messages)
  • Even though users may decide not to start scoped span (not to call WithSpan on tracer), Activity will be there (it always flows - you can explicitly prevent it only). In this case, if users keep using Spans without scopes and will propagate SpanContext explicitly - everything will still be working for them, they will simply ignore Activites.

Can Activity be created only when WithActivity called?

@SergeyKanzhelev
Copy link
Member

I glanced over files - there are so many needed changes!

@megakid
Copy link

megakid commented Jul 4, 2019

Note that in .NET Core 3.0, the entire HTTP stack will natively support w3c tracing standards + their automatic propagation, see https://twitter.com/davidfowl/status/1144127100997128197?s=21

@lmolkova
Copy link
Author

lmolkova commented Jul 4, 2019

Note that in .NET Core 3.0, the entire HTTP stack will natively support w3c tracing standards + their automatic propagation, see https://twitter.com/davidfowl/status/1144127100997128197?s=21

Yes, and it's done with Activities

@lmolkova
Copy link
Author

lmolkova commented Jul 6, 2019

@SergeyKanzhelev thanks for the feedback, I'll think more if we can create Activity on-demand only. On one side this is default OpenTelemetry behavior, on the other side - .NET philosophy is Activity everywhere all the time. Also users won't do explicit WithActivity and it will lead to problems like if they create spans manually, it will break all autocollectors (that work over Activity if you want it or not).

I'm going to close it and split into multiple smaller PRs.

#133: aligning SpanBuilder and Tracer with spec
#136 switching to ActivitySpanId and ActivityTraceId
Activity support is coming after that - we can continue the discussion here in the meantime

@lmolkova lmolkova closed this Jul 6, 2019
@lmolkova lmolkova mentioned this pull request Jul 9, 2019
@lmolkova lmolkova deleted the lmolkova/supportActivities branch December 19, 2019 05:35
Yun-Ting pushed a commit to Yun-Ting/opentelemetry-dotnet that referenced this pull request Oct 13, 2022
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.

3 participants