Remove lazy Event and Link API from Span interface

[arminru]

The SDK only supports head-based sampling and spans expose an IsRecording flag. Checking this one is more effective in avoiding unnecessary work than building an event or link constructor that is lazily invoked, so the lazy API doesn't really help solving any use case. This also cleans up the API surface.

Some (most?) language implementations haven't implemented this MUST/SHOULD requirement yet and will benefit from removing it from the spec. Those who have implemented it (I noticed that at least Python offers an add_lazy_event API) can either drop it or take the liberty to offer it as an improvement which would be an implementation detail. We could, however, also document it as an optional feature in the spec but IMHO that's not worth mentioning, I'd rather emphasize and encourage IsRecording to be checked by users rather than pointing them to a lazy API. Happy to hear other opinions on this, however.

I also noticed that the API to add links at span creation is specified as a should but the first sentence in that section says:

During the Span creation user MUST have the ability to record links to other Spans.

Therefore I'm quite sure that this was actually intended as a MUST and fixed it along the way. I'll carve it out of this PR if it turns out to be controversial, however.

Also obsoletes #533.

[bogdandrutu]

I am ok with this, we can decide to add later if we need because adding this new API should be backwards compatible.

[arminru]

I am ok with this, we can decide to add later if we need because adding this new API should be backwards compatible.

@bogdandrutu I agree. In this case, if the language implementations that have already implemented this API don't remove it before GA, we would have to bring back the very same definition to avoid a breaking change, however.

[tigrannajaryan]

I agree, if we are not sure it is needed better to remove it from the spec now and we can add later if needed.

[bogdandrutu]

@arminru are you suggesting to make everyone remove it?

[arminru]

@bogdandrutu As mentioned above I'd leave that open as an implementation detail but if we later (re-)specify a lazy API it would either have to match the current implementation(s) or would constitute a breaking change (or a mess if both versions would be kept in parallel). If it's needed, just re-adding the current spec should be fine, however, so that will most likely not be an issue anyway.

[bogdandrutu]

So I think we should recommend implementation to remove it for the moment unless it is too big of a problem.

[arminru]

I had a brief look at

• Python (https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/trace/span.py#L55-L65)
• Ruby (https://github.com/open-telemetry/opentelemetry-ruby/blob/master/api/lib/opentelemetry/trace/span.rb#L69-L71)
• Java (https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/Span.java)
• C# (https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Api/Trace/TelemetrySpan.cs)
• Go (https://github.com/open-telemetry/opentelemetry-go/blob/master/api/trace/api.go)
• JS (https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-api/src/trace/span.ts)
• C++ (https://github.com/open-telemetry/opentelemetry-cpp/blob/master/api/include/opentelemetry/trace/span.h)
• Erlang (https://github.com/open-telemetry/opentelemetry-erlang/blob/master/src/ot_span_ets.erl)
• Rust (https://github.com/open-telemetry/opentelemetry-rust/blob/master/src/api/trace/span.rs)
• PHP (https://github.com/open-telemetry/opentelemetry-php/blob/master/api/Trace/Span.php)
• Swift (https://github.com/open-telemetry/opentelemetry-swift/blob/master/Sources/OpenTelemetryApi/Trace/Span.swift)

and it looks like if only Python currently support adding events lazily. I couldn't find any API to add links lazily at all.

@open-telemetry/python-maintainers
Please consider removing the lazy methods from the API if this PR gets accepted.

@open-telemetry/ruby-maintainers
I couldn't find any definition or implementation of the tracer.create_event method mentioned for creating events lazily so the docs might be outdated here.

[bogdandrutu]

@arminru Java has that for example by implementing the Event interface https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/Span.java#L194

Probably some others have the same.

[yurishkuro]

why does this say "Span creation"? This section looks like adding links to already created spans. Span creation is covered in previous section, L272.

[arminru]

It just specifies in more detail how the API is supposed to look like (i.e., accepting both properties as arguments).

[Oberon00]

@yurishkuro This is a subsection of that previous section (since links can only be created at span creation time).

[carlosalberto]

@arminru Rebase on Monday (or next week), so we can merge (we have enough approvals, and all issues are solved, if I'm correct).

[Oberon00]

@yurishkuro This is a subsection of that previous section (since links can only be created at span creation time).

[bogdandrutu]

Not to block this PR:

and it looks like if only Python currently support adding events lazily. I couldn't find any API to add links lazily at all.

but pointed previously that this is not a correct statement.

Does this change things here?

[arminru]

@bogdandrutu Not to the PR itself, I think.
If we want the other SIGs to double-check their implementation and remove the feature, if already implemented, we'll have to get maintainer's attention on this. Unfortunately we don't have a proper process for that yet and merely hope for all SIGs to regularly check the spec for changes. Adding an explicit "any former lazy API shall be removed" to the spec would sound odd to me, though. Could you mention this PR up in the maintainer's sync meeting once merged?

[carlosalberto]

@arminru I started to put a list of changes in the Maintainers list, so the owners are reminded of any recent change they need to make (e.g. ProbabilitySampler -> TraceIdRatioBasedSampler). So, yes, let's take that path.

[arminru]

@carlosalberto @bogdandrutu I added a more explicit note for the SIGs to the changelog in d8d6a21.

[arminru]

@carlosalberto If we have an established process for assuring spec changes are reflected in implementations now, it would be nice if you could document that on open-telemetry/community#317 and close the issue. Thanks!