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

Naming conventions confusion #1985

Closed
mdrgeniebelt opened this issue Apr 15, 2021 · 10 comments
Closed

Naming conventions confusion #1985

mdrgeniebelt opened this issue Apr 15, 2021 · 10 comments
Labels
question Further information is requested

Comments

@mdrgeniebelt
Copy link

Question

Hey, first I want to say that you people are doing great job with OT :) I really appreciate all the effort.

I have few questions though. Reading the documentation and going through examples I'm getting more and more confused on which API I should be using.

I'm not really super familiar with ActivitySource etc so the terminology coming from open telemetry speaks a bit more to me (especially that it is language-agnostic - I can go to someone doing OT in Go, nodejs etc and speak about spans and traces but we would not understand each other if I started speaking about ActivitySource, Activities and so on).

So I wonder...why do we drag the old names into implementation of OT in .net? Wouldn't it help if we had one unified API with names as in specification?

For instance, in the documentation for Baggage it states:

Baggage API allows users to add context to metric, traces, and logs. Baggage can be propagated out of proc using Propagators. OpenTelemetry SDK ships a BaggagePropagator and enables it by default.

So I started using Baggage.Current.SetBaggage etc...but then I saw somewhere that someone else was using Activity.Current?.Baggage and I started wondering...is it the same baggage or something different, that I shouldn't use? In fact if I set something in Baggage.Current.SetBaggage it's not being propagated to Activity.Current?.Baggage...ok then, so I should be using the Baggage.Current?

Then we have the ActivitySource which from my understanding is equal to Tracer from OT library (?) and with instance of that Tracer I can create TelemetrySpan, right? Which in fact is just wrapping Activity?

I don't know if it's only me - being so confused about it, but it really seems like by dragging activity source into these libraries it makes everything blurry, especially for newbies when it comes to OT like me :)

@mdrgeniebelt mdrgeniebelt added the question Further information is requested label Apr 15, 2021
@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Api/README.md#introduction-to-opentelemetry-net-tracing-api Please take a look here which briefs the decision to use Activity, vs re-building a new API.
(Span is represented by Activity, Tracer by ActivitySource)

There is an existing Baggage API on Activity, which existed long before Otel and is not complying to Otel specs. Hence Otel users are not advised to use it, as mentioned in this doc about Baggage:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Api/README.md#baggage-api

@mdrgeniebelt
Copy link
Author

Hi @cijothomas thanks for the answer :) Yeah I read it few times when I was trying to understand how to configure and use these libraries.

I understand that using Activity API underneath makes sense but...you're already building new API on top of it (with Tracer and Spans) so why not promote it as a main terminology in documentation? :) Maybe I'm wrong but I think it would be easier for people coming from different frameworks and languages, jumping into .net and being able to just understand the terminology for things like opentelemetry.

@cijothomas
Copy link
Member

cijothomas commented Apr 20, 2021

you're already building new API on top of it (with Tracer and Spans) so why not promote it as a main terminology in documentation? :)

If you are referring to the shim, then it was introduced to help people who preferred the Otel names (Tracer, Span instead of ActivitySource, Activity). The recommendation is to always use the .NET APIs directly. Long term, there will be no shim in this repo, instead we expect every API to be covered by .NET runtime itself. (as .NET will keep adding features to limit the gaps with Otel.)

Were you suggesting to use "Activity (Span in Otel terminology" instead of "Activity" in the docs? Happy to get more feedback on how to make the docs better.

@cartermp
Copy link
Contributor

@cijothomas So in this case the expectation is that .NET adds APIs for otel-compliant baggage and context propagation in the runtime?

Long term, there will be no shim in this repo, instead we expect every API to be covered by .NET runtime itself. (as .NET will keep adding features to limit the gaps with Otel.)

That won't be portable unless it's done as an out-of-band package, in which case it's just needless code churn. But frankly I think it's also a little concerning that removing the shim is being considered now that everything is 1.0.

Happy to get more feedback on how to make the docs better.

The chief problem to consider here is that modern .NET developers work in systems that use more than .NET. Any .NET services instrumented with Activity-based APIs now has an additional learning curve, where all team members need to remember the ".NET version" of OTel concepts. Not to mention that most OTel backends use terminology like "span" and "trace", and so .NET developers will have to make that mental leap each time they use a tool.

@cijothomas
Copy link
Member

So in this case the expectation is that .NET adds APIs for otel-compliant baggage and context propagation in the runtime?

Yes, as a general direction.
There were some changes in this direction in .NET 6.0 -https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DistributedContextPropagator.cs

The baggage is still an issue (in the sense .NET runtime doesn't provide OTel compatible baggage). These are things we'll gather more feedback and propose to be added in .NET runtime itself. We'll be opening specific issues here to track each of these issues.

That won't be portable unless it's done as an out-of-band package, in which case it's just needless code churn

I am not sure I follow the above comments. What exactly is meant by "not portable", "code churn" ?
The API's required by Otel are part of System.Diagnostics.DiagnosticSource package, which is an out-of-band package, supporting very old .NET versions, not just the newest.

But frankly I think it's also a little concerning that removing the shim is being considered now that everything is 1.0.

Don't be concerned. We follow semver versioning as documented here. Since the SHIM is part of 1.x stable release, it won't be removed at least until 2.0. (To the best of my knowledge, 2.0 is not even planned!).
If we ever remove the SHIM, we'll only do so after opening issues, and gathering enough feedback before proceeding. If there is enough demand in keeping SHIM, it'll be kept.

@cijothomas
Copy link
Member

modern .NET developers work in systems that use more than .NET. Any .NET services instrumented with Activity-based APIs now has an additional learning curve, where all team members need to remember the ".NET version" of OTel concepts.

Yes, the above was one of the reason which led to the Shim component in 1.0.

The "different in terminology" is documented here for tracing. For Metrics, the difference is even smaller. If there are specific areas where doc improvement or shim or something else can help - happy to address that.

@cartermp
Copy link
Contributor

cartermp commented Dec 1, 2021

@cijothomas Thanks, that makes sense. In general I think the shim is great (coming from otel in another language is easy and there's no suprises!)

I am not sure I follow the above comments. What exactly is meant by "not portable", "code churn" ?

I wasn't aware this was all possible solely within System.Diagnostics.DiagnosticsSource. That's great! My concern was that it might require changes in the runtime itself, in which case it wouldn't be portable, and might require an additional package to do things like use otel-compliant baggage. But it sounds like that won't be needed.

Speaking of docs, one thing I'm looking to help improve is the overall docs/guidance for SDKs. Put together a scorecard that's being tracked here as well. Would love any feedback on that process and any considerations for .NET in particular, since a part of it would be guidance on what to use based on scenario.

@ericsampson
Copy link

ericsampson commented Dec 1, 2021

As an end user, I feel that the OTel terms are the primary ones that should be emphasized, so please don't get rid of the shim.

Like Phillip mentioned, companies are polyglot these days (we run services in Node, .NET, Go, and Rust) and so using the OTel terminology helps greatly when devs talk to each other, move back and forth between teams and languages, etc. And also makes documentation easier to understand, both for vendors like Honeycomb and also for companies writing internal dev docs.
The fact that this library is implemented using the .NET Activity APIs is just that--an implementation detail, and IMO preferably wouldn't have been exposed that at all in this library :)

Cheers,
Eric

@mdrgeniebelt
Copy link
Author

mdrgeniebelt commented Dec 2, 2021

I'd just like to emphasise how much I agree with both @cartermp and @ericsampson.

I started my journey with OTEL by reading the spec (not all of it course) and it made sense to me, then I moved to this repository and its documentation and I got very confused. Working with other team members that use nodejs I had issues constantly switching between spans, traces, activities, baggage ('which one are we talking about? Activity baggage or the other one?').

So like @ericsampson mentioned, the fact that .NET otel package is using activities - is an implementation detail, something that would be 'interesting' for people writing their own otel libraries but not really interesting for consumers of these libraries.

@cijothomas
Copy link
Member

Closing old issue as there are no open questions. Feel free to re-open, or create new issue for further clarifications.

(There is no shim removal tracked by any issues, so its not even in the radar for now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants