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.

Sign up for GitHub

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
Closed

add finishing the currently active span to tracer operations #516

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6eda2a2
add finishing the currently active span to tracer operations
tsloughter Mar 14, 2020
247d453
use End intead of Finish for tracer operation
tsloughter Mar 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion specification/api-tracing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

- Get the currently active `Span`
- Make a given `Span` as active
- Make a given `Span` active

The `Tracer` MUST internally leverage the `Context` in order to get and set the
current `Span` state and how `Span`s are passed across process boundaries.
Expand Down