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

add finishing the currently active span to tracer operations #516

Closed
wants to merge 2 commits into from

Conversation

tsloughter
Copy link
Member

The current API Tracing spec includes:

When an active Span is made inactive, the previously-active Span SHOULD be made active.

but has no Tracer operation defined that would result in needing this defined. I believe it was intended for specifying what happens when the Tracer ends a Span, which is the addition made in this commit.

Similar to my PR #485 I believe this change reflects OTEP-66 and operating on the context, as an example in the otep shows: Tracer::EndSpan(context).

It may be better under MUST but I put it under SHOULD for now.

@@ -126,8 +126,9 @@ The `Tracer` MUST provide functions to:

The `Tracer` SHOULD provide methods to:

- End the currently active `Span`
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't call "active context management" part of the Tracer's responsibilities. I'd call it part of the context library's responsibilities. If there's something managing which context is "active", then it should provide interfaces to start-new-active-span-scope and to leave-scope. These are not Tracer interfaces, though they will use a tracer to start the new span, switching to the previous active span is not a tracer responsibility IMO.

In a C++ API, for example, you might have something like:

   {
     ScopedSpan span = context.StartSpan(tracer);
     // ... blah blah
     
     // span ends here implicitly when the ScopedSpan destructor runs. It restores the previous context and 
     // ends the span. 
  }

These are not Tracer APIs, this is a context-management API that implements active span state (IMO).

Copy link
Member Author

Choose a reason for hiding this comment

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

There has been talking about doing all this through the context library but it was put off to get a release out. So right now as far as I know Tracer is responsible for this and will be for the for see-able future.

The spec also already says switching to the previous span is done by the tracer.

Things like OnEnd Span Processors are tied to a Tracer, so just like with starting a span it makes sense that ending a span is doing through the Tracer, which runs the processors.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Tracer gave you a Span, so Span.End should be sufficient, I don't see why a Tracer function is required. You may be taking a position on open-telemetry/oteps#68. I am against that proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would mean for some languages that every Span has to carry around processor information so that span.End(Span) knows what to do. That or keep the name/version of the tracer in each Span so it can lookup the Tracer and then call the processors.

It all seems to be pushing stuff into the Span just so the Tracer isn't used to End Spans...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes because the Span is the object that is ending. Doesn't the Span have a reference to the Tracer?

In #514 there is a lengthy discussion about "end" vs "finish", but if you follow the argument there, this proposal makes end into a transitive verb (i.e., the Tracer is Ending the Span). I believe it's even less appropriate to use End as a transitive verb than it is to use End as an intransitive verb (as we are currently doing).

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't all have the ability to use references :), so it requires copying the whole tracer structure, or just the processors into the span or using a "reference" which is really just the name/version of a tracer so it has to be looked up from a global store when ending.

All of which just to not use the Tracer to finish the Span, when clearly the Tracer is required to finish the span since all this has to be done so that the Span can access the Tracer to finish itself.

@tsloughter
Copy link
Member Author

I believe the decision at the last spec meeting was to close this with the views being that most Spans can have references to the tracer and then use that reference to get what is needed to end a span. And that in cases that references aren't an option the tracer can instead be used directly.

I do see some benefit to this reference to the tracer pattern because it guarantees the same tracer ends the span that started it.

@tsloughter tsloughter closed this Apr 4, 2020
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