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

Improvements to the System.Diagnostics.Activity APIs #98

Merged
merged 29 commits into from
May 21, 2020

Conversation

noahfalk
Copy link
Member

Discussion was getting hard in dotnet/runtime#31373 and some of the collaborators also felt that we were circling on some issues. The hope is that this PRs to this design doc will allow easier inline commenting, threaded conversations, and serves as a more durable public recording of decisions.

@tarekgh - Despite me creating the PR I am still thinking of you as its owner. Please update it (or replace it) however you think is useful, similar to what you would have done for the original issue. I didn't wait for you to create it solely for expediency and because I didn't think you'd mind.

@marek-safar
Copy link
Contributor

/cc @lateralusX

AndreyTretyak added a commit to microsoft/Omex that referenced this pull request Mar 19, 2020
- Move all Activty processing into extensible activity observres
- Add logic for supporting guid correlation id
- Change activity API to correspond to future activity changes dotnet/designs#98
- Switch to using BacgroungService for Omex SF services to reduce the amount of custom code
- Now all core for using activities inside Abstraction package, add TimedScopes package needed only to log activities
Clarify that we have an explicit goal to make Activity the representation of OT span on dotnet.
Better terminology: Microsoft is participating in OT
@tarekgh
Copy link
Member

tarekgh commented Apr 22, 2020

@kadukf we don't provide meta-registry with the APIs. The way to test that is to execute the test in a separate process. You may look at the tests https://github.com/dotnet/runtime/pull/35220/files#diff-4f6304f8e96611ddade3f18538f82f2a as an example of doing that.

@noahfalk
Copy link
Member Author

Another alternative for testing is to use dependency injection for the ActivitySource. If each test case or group of tests uses a different ActivitySource to produce Activities then ActivityListeners will be able to subscribe to just those sources and not any other sources.

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added few questions. Apologies if those were already discussed earlier.

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for clarifying all questions.
Looking forward to preview bits!

proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
proposed/activity-improvements.md Outdated Show resolved Hide resolved
tarekgh and others added 9 commits May 11, 2020 09:58
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
@MikeGoldsmith
Copy link

Sorry I'm late to the discussion.

I have some reservations regarding replacing the OpenTelemetry span and tracer interfaces with the proposed Activity & ActivitySource.

  • Why doe we need to supersede the span / tracer API with Activity & ActivitySource? They form a series of deviations from the OpenTelemetry specification, eg non-string tags, nullable activity vs no-op span, span processing pipeline, exporter pipeline, etc.

  • Would it be possible to achieve the same level of backwards compatibility & integration with Activity / ActivitySource with a OpenTelemetry span translation?

  • The proposal states ActivityContext depends on the W3C TraceContext implementation - however OpenTelemetry needs to support other propagation formats, eg B3. Many current implementations depend on a mixture of propagation formats (eg B3) and omitting B3 (or not allowing custom context) means there is no way to support those applications that propagate with anything other than TraceContext.

@noahfalk
Copy link
Member Author

Thanks for the feedback @MikeGoldsmith!

Why doe we need to supersede the span / tracer API with Activity & ActivitySource? They form a series of deviations from the OpenTelemetry specification,

It looked better than the alternatives:

  1. If .NET defined Tracer and Span types as originally specified in a separate OpenTelemetry library, the feedback we got from some .NET library authors was they would not take a dependency outside the framework APIs. Either they would leave their libraries uninstrumented or they would build custom instrumentation and the onus would fall on the OpenTelemetry developers to write auto-collectors.
  2. If the .NET Framework took the Tracer and Span APIs exactly as specified and put them in the framework we'd now have two competing APIs for distributed tracing that aren't fundamentally that different. We didn't want the confusion and balkanization that comes with multiple APIs if it didn't bring considerable additional value. My assumption is that most .NET developers care a fair amount about .NET APIs being self-consistent but care relatively little if the .NET API has a few discrepancies as the related OpenTelemetry API in python, java, or node.

eg non-string tags

This one is probably resolvable in the API and not a fundamental limitation of picking this direction

nullable activity vs no-op span

That one is more fundamental. I am guessing most .NET developers will make their judgement about this design choice based on ease of use or performance, not by spec/cross-language comparison.

span processing pipeline, exporter pipeline

I'm not sure what the concerns are in this area? Other than Span and Activity having different names I would guess the work to build an export pipeline is largely the same. If there are obstacles I'd like to understand them.

Would it be possible to achieve the same level of backwards compatibility & integration with Activity / ActivitySource with a OpenTelemetry span translation?

I expect someone could build a decent adapter on top of the new Activity APIs at least. The main challenges I'd anticipate adding an adapter are:

  1. Performance - Mapping between different abstractions takes memory allocations and CPU cycles
  2. Simplicity - Having multiple API standards is very likely to create ecosystem confusion, conflicting guidance, and fragmented use.

The proposal states ActivityContext depends on the W3C TraceContext implementation - however OpenTelemetry needs to support other propagation formats

Perhaps this is just an issue with wording in the proposal text? ActivityContext is designed to be .NET's implementation of OT SpanContext. It has the prescribed SpanContext field names and types, aside from IsRemote which we were told was being likely to be removed from the spec.
Is your concern that the APIs here don't have an implementation of Propagators? So far it wasn't clear that this needed a runtime implementation as opposed to implementing it in the OpenTelemetry SDK library. We do also have a queued work item to look more closely at distributed context support so it is possible some propagator type rise as a requirement there.

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.