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 all 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
63 changes: 59 additions & 4 deletions 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 parameter.

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 and previous state should be restored when the returned object is closed.

#### `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 in the span population with
the values that will allow correlation of telemetry is also a caller responsibility.

This API should be non-blocking.

#### `GetBinaryFormat`: returns the binary format interface which can serialize/deserialize `Span`s.

There should be no parameter.

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 interface which can inject/extract `Span`s.

There should be no parameter.

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 Expand Up @@ -50,14 +105,14 @@ recording status, none of the below may be called after the `Span` is finished.

#### `GetContext`: retrieve the `Span`s `SpanContext`

There should be no parameters.
There should be no parameter.

**Returns** the `SpanContext` for the given `Span`. The returned value may be
used even after the `Span` is finished.

#### `IsRecordingEvents`: returns the flag whether this span will be recorded

There should be no parameters.
There should be no parameter.

Returns true if this `Span` is active and recording information like events with
the `AddEvent` operation and attributes using `SetAttributes`.
Expand Down Expand Up @@ -136,7 +191,7 @@ with the `Span`).
Call to `End` of a `Span` MUST not have any effects on child spans. Those may
still be running and can be ended later.

There MUST be no parameters.
There MUST be no parameter.

This API MUST be non-blocking.

Expand Down