Add Tracer operations specs. #105

songy23 · 2019-06-11T18:55:56Z

Fixes #38.

Documents are based on Java API: https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/Tracer.java.

specification/tracing-api.md

reyang · 2019-06-11T19:25:32Z

specification/tracing-api.md

+Note, the `SpanContext` object on the span population with
+the values that will allow correlation of telemetry is also a caller responsibility.
+
+#### `GetBinaryFormat`: returns the binary format of `Span`s.


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

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

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

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?)

If today we have BinaryFormat and HttpTextFormat, tomorrow we discovered that we need MqttMetadataFormat or YetAnotherFormat, do we end up adding APIs to Tracer?

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).

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.

reyang · 2019-06-11T19:27:43Z

specification/tracing-api.md

+
+Required parameters:
+
+- `SpanData` to be reported to all exporters.


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

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

specification/tracing-api.md

songy23 · 2019-06-11T23:05:37Z

/cc @c24t @dinooliva @mayurkale22 @rghetia

jmacd · 2019-06-11T23:12:46Z

specification/tracing-api.md

+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.


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().

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.

specification/tracing-api.md

mayurkale22 · 2019-06-11T23:22:15Z

specification/tracing-api.md

+
+The scope is exited when the returned object is closed.
+
+#### `SpanBuilder`: returns a `SpanBuilder` to create and start a new `Span`.


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

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.

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 ;)

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.

rghetia · 2019-06-12T00:50:15Z

specification/tracing-api.md

+
+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.


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

That's correct, updated.

rghetia · 2019-06-12T01:05:23Z

specification/tracing-api.md

+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.


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

It returns the format interface. Updated to make it clear.

reyang · 2019-06-12T04:19:31Z

specification/tracing-api.md

+
+Required parameters:
+
+- The `Span` to be set to the current context.


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

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

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

This is responsibility of the user ;)

+1

specification/tracing-api.md

Fixes open-telemetry#38.

songy23

PTAL

songy23 · 2019-06-12T17:11:21Z

specification/tracing-api.md

+
+Required parameters:
+
+- The `Span` to be set to the current context.


This is responsibility of the user ;)

+1

songy23 · 2019-06-12T17:11:31Z

specification/tracing-api.md

+
+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.


That's correct, updated.

specification/tracing-api.md

songy23 · 2019-06-12T17:12:37Z

specification/tracing-api.md

+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.


It returns the format interface. Updated to make it clear.

reyang

Looks good in general, we need to revisit the GetXyzFormat, SpanBuilder and GetActiveSpan APIs later.

songy23 · 2019-06-12T20:48:58Z

@SergeyKanzhelev Please help me merge this PR. Remaining issues will be discussed separately.

specification/tracing-api.md

Co-Authored-By: Mayur Kale <[email protected]>

* Add Tracer operations specs. Fixes open-telemetry#38. * recordSpanData should be async. * Fix typos. * Fix review comments. * Fix another typo. Co-Authored-By: Mayur Kale <[email protected]>

Signed-off-by: Bogdan Drutu <[email protected]> Co-authored-by: Carlos Alberto Cortez <[email protected]>

* Add Tracer operations specs. Fixes open-telemetry#38. * recordSpanData should be async. * Fix typos. * Fix review comments. * Fix another typo. Co-Authored-By: Mayur Kale <[email protected]>

songy23 requested review from AloisReitbauer, bogdandrutu, c24t, carlosalberto, iredelmeier, reyang, SergeyKanzhelev, tedsuo and yurishkuro as code owners June 11, 2019 18:55

reyang reviewed Jun 11, 2019

View reviewed changes

specification/tracing-api.md Show resolved Hide resolved

reyang reviewed Jun 11, 2019

View reviewed changes

songy23 force-pushed the tracing-operations branch from 1ffa28b to 320f63a Compare June 11, 2019 21:59

SergeyKanzhelev reviewed Jun 11, 2019

View reviewed changes

specification/tracing-api.md Outdated Show resolved Hide resolved

SergeyKanzhelev approved these changes Jun 11, 2019

View reviewed changes

jmacd reviewed Jun 11, 2019

View reviewed changes

mayurkale22 reviewed Jun 11, 2019

View reviewed changes

specification/tracing-api.md Outdated Show resolved Hide resolved

mayurkale22 reviewed Jun 11, 2019

View reviewed changes

songy23 mentioned this pull request Jun 11, 2019

API: Should we move the current span API to be part of Scope? #108

Closed

songy23 force-pushed the tracing-operations branch from fc78453 to 37b4b24 Compare June 11, 2019 23:30

rghetia reviewed Jun 12, 2019

View reviewed changes

reyang reviewed Jun 12, 2019

View reviewed changes

specification/tracing-api.md Outdated Show resolved Hide resolved

songy23 mentioned this pull request Jun 12, 2019

API: Should propagating formats be part of Tracer or not? #112

Closed

songy23 added 3 commits June 12, 2019 10:08

Add Tracer operations specs.

6a5f3b7

Fixes open-telemetry#38.

recordSpanData should be async.

7c75528

Fix typos.

6b774d3

songy23 force-pushed the tracing-operations branch from 17b60cd to 6b774d3 Compare June 12, 2019 17:08

Fix review comments.

16fc6c0

songy23 commented Jun 12, 2019

View reviewed changes

reyang approved these changes Jun 12, 2019

View reviewed changes

mayurkale22 reviewed Jun 12, 2019

View reviewed changes

specification/tracing-api.md Outdated Show resolved Hide resolved

Fix another typo.

c534f4e

Co-Authored-By: Mayur Kale <[email protected]>

mayurkale22 approved these changes Jun 12, 2019

View reviewed changes

SergeyKanzhelev merged commit c186f8d into open-telemetry:master Jun 13, 2019

songy23 deleted the tracing-operations branch June 17, 2019 17:26

rockb1017 pushed a commit to rockb1017/opentelemetry-specification that referenced this pull request Nov 18, 2021

Version components, not repository (open-telemetry#105)

dbbf576

carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 21, 2024

Move trace otep to trace directory (open-telemetry#105)

82a87b6

Signed-off-by: Bogdan Drutu <[email protected]> Co-authored-by: Carlos Alberto Cortez <[email protected]>

carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 23, 2024

Move trace otep to trace directory (open-telemetry#105)

42af2e0

Signed-off-by: Bogdan Drutu <[email protected]> Co-authored-by: Carlos Alberto Cortez <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tracer operations specs. #105

Add Tracer operations specs. #105

songy23 commented Jun 11, 2019

reyang Jun 11, 2019

songy23 Jun 11, 2019

reyang Jun 12, 2019

carlosalberto Jun 12, 2019

songy23 Jun 12, 2019

reyang Jun 11, 2019

songy23 Jun 11, 2019

songy23 commented Jun 11, 2019

jmacd Jun 11, 2019

songy23 Jun 11, 2019

mayurkale22 Jun 11, 2019

songy23 Jun 11, 2019

carlosalberto Jun 12, 2019 •

edited

Loading

songy23 Jun 12, 2019

rghetia Jun 12, 2019

songy23 Jun 12, 2019

rghetia Jun 12, 2019

songy23 Jun 12, 2019

reyang Jun 12, 2019 •

edited

Loading

carlosalberto Jun 12, 2019

SergeyKanzhelev Jun 12, 2019

songy23 Jun 12, 2019

songy23 left a comment

songy23 Jun 12, 2019

songy23 Jun 12, 2019

songy23 Jun 12, 2019

reyang left a comment

songy23 commented Jun 12, 2019


		Required parameters:

		- `SpanData` to be reported to all exporters.


		The scope is exited when the returned object is closed.

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


		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.


		Required parameters:

		- The `Span` to be set to the current context.

Add Tracer operations specs. #105

Add Tracer operations specs. #105

Conversation

songy23 commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

songy23 commented Jun 12, 2019

carlosalberto Jun 12, 2019 •

edited

Loading

reyang Jun 12, 2019 •

edited

Loading