Comments (19)
@lmolkova yes I really like the way that .NET is going with this, I hope other language runtimes eventually catch up. :)
I'm not convinced that this would be a major change to the current implementations in other languages, even though it changes the API significantly.
For each language, it would need to be done as a very complete PR, with the edge cases handled and benefit shown; and not dribbled out. I would not want to block the main line of SDK work with this.
from oteps.
@yurishkuro Josh means that some gophers refuse to store a reference to a Context. So if you take away their Span handle, they have nothing to store but the Context, and they are sad. This comes from a literal reading of the following advice from https://golang.org/pkg/context/
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx
I won't go into it here, but there are enough edge cases that a literal interpretation of this advice is silly. Occasionally, you have to store a context for use later.
@carlosalberto this is the old example from OpenTracing Java. It's easy to see how this change would make the instrumentation more readable, in just about every language: https://docs.google.com/presentation/d/1FKnzs19iuJyDIy0cHffgC8H1B4y_U4sXDw7N_YswnQw/edit
from oteps.
There may be cases where it is necessary to have a Span object, for example an interceptor pattern like this:
Interceptor {
onStart(Headers);
Context beforeExecutedHandler(Context);
onEnd();
}
The Interceptor
implementation needs to create a Span
in the onStart
and install the Span
in the Context later. With the proposed API this is not possible because it is not easy to "merge" contexts in the beforeExecutedHandler
.
So I would definitely not rush a decision like this before v1.
from oteps.
Is this proposal specific for Go and if not, how it could work for other languages?
It seems to imply that all spans have to be active and that language allows to rely on current spans in all cases. I don't think this is the case for Java or C++. Even for languages with full async support, it's not always possible to use the current span reliably.
from oteps.
First, I think this issue belongs more to the spec repository than the OTEP repository (even though an OTEP may come out of it).
Second, as I see it the main change here is that for each Span operation you now have the Tracer as an additional input (you could just implement Span as a reference to SpanContext and a Tracer and have the public methods of Span delegate to Tracer methods of the form you suggested). Is that correct?
This has implications for the SpanContext too, because we probably would want some additional property like int activeSpanIndex
otherwise we force something like a HashMap access for each Span operation. Then the question is who has (read or write) access to this property? Should it be defined in the SDK or API? If in the SDK, we are back to #58. And so on.
from oteps.
This feels like a Go-specific proposal to me, and it's one that's been discussed in the past I'm sure many times. The question is, even if every operation on a Span is equivalent to some Tracer operation on a context, should we still have a Span type? I can be convinced not, in this case, but there's a specific line in the Golang Context documentation that causes some concern:
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.
I personally disagree with the advice not to store Contexts, but as a compromise to some of my co-workers, whenever I've done so I embed it in another struct and clearly document the behavior of the stored context, i.e., never store a bare Context, only store contexts in derived Context structures with a clear intent. Then, we can remove the Span type.
from oteps.
you could just implement Span as a reference to SpanContext
Yes, but that's the point - if it can be done, why have the Span object in the API?
I agree that we'd need to revisit #58.
@jmacd I didn't follow your train of though. How is this related to storing Context anywhere? Context is a propagation mechanism, which is independent of Span vs. SpanContext duality. The code examples I had are Go specific, but in another language you can just replace ctx
with spanCtx
.
from oteps.
I remember that we mentioned back in OpenTracing having an abstraction layer (working on top of the OT api) that effectively would hide these details, so you would always work with the current Span
, as depicted in your example. I feel this approach would be great, as it would simplify things for 'end users', but wouldn't probably help with optimizations like the Span
pooling you mentioned ;)
from oteps.
I fully support this proposal, also I'm prepared to stand before any and all gophers and state that storing Contexts is the right thing to do. I appreciated the related discussion here: https://github.com/open-telemetry/oteps/pull/66/files#r347198758
from oteps.
I'm a +1 on this. It at least applies to Erlang/Elixir, so not only Go.
It is also basically how the Erlang impl already works. We provide an additional interface that sits on top of the official API, a module named otel
.
We also don't allow modifications to a span after it has been finished. A finished span is moved to a new buffer and out of the active spans buffer. I'm guessing this is related to what @yurishkuro is talking about with simplifying memory management.
Actually, yes:
The tracer can keep a buffer of active spans indexed by immutable span contexts that are kept by the user code.
This is exactly how the Erlang impl works. And when end_span
is called the span is removed from the active spans and moved to a separate buffer that is used by the batch processor (assuming the batch processor has been added as a span processor).
from oteps.
There was some discussion about this in a call about Context Propagation today. I would be satisfied with a specification update stating that Span
objects are logical constructs which refer to a span context. The notion of "CurrentSpan" returning a span is really just semantic sugar for a type which encapsulates a span context. I believe this addresses your concern, without needing to actually remove Span
from the specification. It would allow a valid implementation to be stateless, it allows a meaningful interpretation for span events that happen after Span
s finish, and so on.
from oteps.
@lmolkova I believe it implies that we can simply when adding a context object to every language. Even if this context object is automatically propagated most of the time, there are cases where the context object must also be instantiated. So, the proposal is that all operations work directly on this context object, rather than first extracting a span from the context.
A pseudocode example, with no automated context propagation:
// operate on the context object directly
SetSpanAttribute(context, "foo", "bar")
// rather than first grab a span handle
span = GetSpan(context)
span.SetAttribute("foo", "bar")
When automated context propagation is available, then the context object is implied.
// use the context directly
SetCurrentSpanAttribute("foo", "bar")
// rather than grab a span handle
span = GetCurrentSpan()
span.SetAttribute("foo", "bar")
Is that a helpful description? I'm not proposing it would look exactly like this in C#, but I do think the result would be a more declarative API, that hides the span object as an implementation detail. I think this leaves more room for improvement in the future, as the API now exposes fewer details. I believe .Net uses activities as the equivalent of "context" in this case.
BTW, it is this OTEP (#66) proposing a separate context layer which is bringing this discussion up. I agree with @bogdandrutu that we have to confirm that edge cases like moving contexts still work. I actually do not think it is complicated to move/merge a span from one context to another, but we would need to create tests and examples for all of these cases first. So I would not want to rush a big change. Perhaps in some cases, this is just sugar to make the API more declarative and take up fewer lines of code.
from oteps.
So my understanding is that this simply eliminates Span interface:
- we have some kind of handler for the span.
- we keep all the Span interface APIs to enrich spans, but expose them through different class (Tracer or whatever)
- We keep readable Span in some shape for exporters
With .NET team we are entertaining the idea of providing all Span capabilities in the platform (which simplifies dependency management enormously). So this is definitely an interesting proposal that helps with this idea.
At the same time, it seems we need something in the SDK to store all active spans (which is not trivial to do right) AND we still ask users to carry handler around AND we keep all the same methods on the different interface.
If we want to do this in v1, I hope to see some real benefits for this choice. I believe there are some, but I'm not sure what is ROI. But after v1, Span interface would need to stay for a while and it might not make sense to do anything about it.
from oteps.
I will vote to keep the Span
type but change the specification for Span
to define it as semantic sugar, making it in every way equivalent to the proposal in this issue but without removing the Span
interface. The original Go "streaming" SDK was implemented this way. A Span was effectively just a context and all operations recorded an event with the context and operation details.
IOW I believe a valid SDK can already avoid the memory management problem -- there is no requirement for an SDK to maintain state in memory to implement a Span. I still believe the specification should be firmed up to say that Span
is semantically equivalent to a context.
from oteps.
I believe this is a very important topic. Please see the related/opposed issue #73.
I believe it's an ergonomic loss to remove the Span API. One of the points of confusion that could arise, were Span
to be removed, is that it becomes easier to mis-use the API by recording a single Span to multiple Tracers (i.e., you could use one Tracer for the Start event, one for the End event).
from oteps.
basically it requires to expose Span APIs on the span context/handler, not the tracer.
There is already enough confusion around current span on the tracer open-telemetry/opentelemetry-specification#108 (is it current for this tracer or current globally).
from oteps.
There is already enough confusion around current span on the tracer open-telemetry/opentelemetry-specification#108 (is it current for this tracer or current globally).
And with this change it is current for the current context, right?
I'd never heard of current span being tied to a tracer or globally, this may be why I've always been so confused in discussions about current/active spans?
I wanted to reiterate that I'm a big +1 on this and really hope the spec goes this way. It is already basically how the Erlang/Elixir implementation works, only basically because to keep the API's for tracer and span modules roughly the same as the spec we have an additional module that acts as a layer above them to combine their APIs.
from oteps.
I think this proposal should receive some serious consideration again now that OTEP 66 (Context OTEP) is merged. Although now, Span should arguably not be replaced by SpanContext (we can remove that too, #73), but OTEP 66's Context + operations on the Tracer.
from oteps.
While we would have had the chance to do this with OTEP66, I think it's too late now. Can we close this?
from oteps.
Related Issues (20)
- Proposal: Reduce clock-skew issues in mobile and other client-side trace sources HOT 5
- Proposal: Supporting Real User Monitoring Events in OpenTelemetry HOT 27
- Proposal: Add Sensitive Data Labels HOT 15
- Proposal: Remote Sampling
- Proposal: "Plugable backend" Tracing Client/Query library HOT 16
- Proposal: Add support for Elastic Common Schema (ECS) in OpenTelemetry HOT 6
- Proposal: specify how opentelemetry will deal with idle metrics no longer being reported
- Add link to opentelemetry.io under About section HOT 1
- Proposal: Support ML Monitoring HOT 6
- Proposal: Dynamic configuration of metrics HOT 2
- Proposal: clarify behavior when retrieving non-existent currently active span HOT 13
- Proposal: The OpenTelemetry Spec should allow SDKs to export all the spans regardless of their sampled flag
- Proposal: OpenTelemetry Sandbox HOT 12
- Proposal: TraceID just being hex value doesnt help with decision making at tracing backend. Is there a possibility to encode additional metadata such as timestamp, region etc. HOT 4
- Proposal: Service renaming HOT 4
- Add a "not implemented" stage to maturity levels HOT 3
- Group maturity status into experimental and stable HOT 2
- How to convert Java Flight Recorder (JFR) file to Profiling Data Model v2 HOT 5
- profiles/follow up: location references in sample HOT 17
- profiles/follow up: consistent time format HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from oteps.