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 Tracer operations specs. #105

Merged
merged 5 commits into from
Jun 13, 2019
Merged

Add Tracer operations specs. #105

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a5f3b7
Add Tracer operations specs.
songy23 Jun 11, 2019
7c75528
recordSpanData should be async.
songy23 Jun 11, 2019
6b774d3
Fix typos.
songy23 Jun 11, 2019
16fc6c0
Fix review comments.
songy23 Jun 12, 2019
c534f4e
Fix another typo.
songy23 Jun 12, 2019
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
57 changes: 56 additions & 1 deletion specification/tracing-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,62 @@ TODO: How tracer can be constructed? https://github.com/open-telemetry/opentelem

### Tracer operations

TODO: Tracing operations. https://github.com/open-telemetry/opentelemetry-specification/issues/38
#### `GetCurrentSpan`: returns the current Span from the current context.

There should be no parameters.

Returns the default `Span` that does nothing and has an invalid `SpanContext` if no
`Span` is associated with the current context, otherwise the current `Span` from the context.
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are more than one Tracer instantiated, potentially with different Resource definitions, can there be more than one current span? I would assume that there's a single current active span.

Therefore, I'm not sure why this should be a Tracer method. If there was an explicit Scope type, as I think there should be, you'd be asking for Scope.Active().

Copy link
Member Author

Choose a reason for hiding this comment

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

If there are more than one Tracer instantiated, potentially with different Resource definitions, can there be more than one current span? I would assume that there's a single current active span.

Agree having one active span makes more sense. Filed #108 for discussion about where the API should live.


#### `WithSpan`: enters the scope of code where the given `Span` is in the current context.

Required parameters:

- The `Span` to be set to the current context.
Copy link
Member

@reyang reyang Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If user passed in a span which has already been closed, what would happen? Unspecified behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so, yeah. This is responsibility of the user ;)

Copy link
Member

Choose a reason for hiding this comment

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

I think SpanContext is available from completed Span. So the only effect would be that all subsequent spans and logs are associated with the Span. All attempts to add links and attributes to the current span will not succeed. But again, it may be legitimate use case to associate telemetry with the completed span

Copy link
Member Author

Choose a reason for hiding this comment

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

This is responsibility of the user ;)

+1


Returns an object that defines a scope where the given `Span` will be set to the current context.

The scope is exited when the returned object is closed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it also restore the previous state after the scope is closed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct, updated.


#### `SpanBuilder`: returns a `SpanBuilder` to create and start a new `Span`.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this (builder pattern) is not gonna applicable for all the languages (ex. NodeJS).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is being discussed at #37. Generally we're using builder pattern to specify which parameters are required when initializing and which can be set after initialization. IMO this doesn't mean all languages need to conform to the builder pattern.

Copy link
Contributor

@carlosalberto carlosalberto Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I have to say also in Python we won't be using the builder pattern - and given that enough languages won't be using this one, we maybe should probably change this so a builder is not mentioned here at all ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm good with replacing builder pattern in the specs and rephrasing it in a more language-agnostic way, as long as we make it clear which parameters are required/immutable vs. which can be mutated. I'd prefer to update it after we reach to an agreement on #37.


Required parameters:

- Name of the span.

Returns a `SpanBuilder` to create and start a new `Span`.

#### `RecordSpanData`: records a `SpanData`.
SergeyKanzhelev marked this conversation as resolved.
Show resolved Hide resolved

Required parameters:

- `SpanData` to be reported to all exporters.
Copy link
Member

Choose a reason for hiding this comment

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

Curious, if the purpose is to export the spandata, why not just call it "export"?

Copy link
Member Author

Choose a reason for hiding this comment

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

My guess is the term export is used elsewhere such as the exporter interfaces in SDK, and we don't want to overload it.


This API allows to send a pre-populated span object to the exporter.
Sampling and recording decisions as well as other collection optimizations are a
responsibility of a caller.

Note, the `SpanContext` object on the span population with
songy23 marked this conversation as resolved.
Show resolved Hide resolved
the values that will allow correlation of telemetry is also a caller responsibility.

This API should be non-blocking.

#### `GetBinaryFormat`: returns the binary format of `Span`s.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to be a Tracer method? It feels like a static method used by propagators.

Copy link
Member Author

Choose a reason for hiding this comment

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

No because we want to allow different implementations for it. If no implementation is provided, API will provide a noop binary format for GetBinaryFormat.

Copy link
Member

Choose a reason for hiding this comment

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

Let's take a step back and consider the following questions:

  1. What is our definition of tracer? How do we decide if something belongs to a tracer or not? (e.g. do we think propagator belongs to the tracer? if we need two propagators, what shall we do?)
  2. If today we have BinaryFormat and HttpTextFormat, tomorrow we discovered that we need MqttMetadataFormat or YetAnotherFormat, do we end up adding APIs to Tracer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this initially belongs to a Tracer given that you are defining propagators for SpanContext, which is tied to the tracing system - that being said, I'm curious about what to do if additional formats are needed (although I'm also curious about how common this is).

(Also, a related item: we had discussed the possibility of composite formats, in case you needed to use text-based formats, such as try B2, then fallback to TraceContext, then fallback to something else, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think @reyang raised valid concerns. I wasn't in the previous discussions on propagators, but in OpenCensus we have an independent package trace.propagation specifically for propagators. Also we allowed getting explicit formats via API calls, like getB3Format vs. getTraceContextFormat.

Anyway I filed #112 for further discussions.


There should be no parameters.
songy23 marked this conversation as resolved.
Show resolved Hide resolved

Returns the binary format for this implementation. If no implementation is provided
then no-op implementation will be used.

#### `GetHttpTextFormat`: returns the HTTP text format of `Span`s.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it return an object or an interface that injects/extracts span context? OR does it return the span context in that format?

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 returns the format interface. Updated to make it clear.


There should be no parameters.

Returns the HTTP text format for this implementation. If no implementation is provided
then no-op implementation will be used.

Usually this will be the W3C Trace Context as the HTTP text format. For more details, see
[trace-context](https://github.com/w3c/trace-context).

## Span

Expand Down