Add Initial OpenTracing compatibility requirements

[carlosalberto]

Fixes #114

This PR tries to mostly describe the current state (mostly in the Java/Python shims). This documented will definitely need to be augmented, once feedback arises after more OT bridges are written (and used). There are a pair of required follow-ups already:

• Formalize the translation of OpenTracing references to span parent and links #562
• Sync and Async children (FOLLOWS_FROM) #65

cc @yurishkuro @codeboten @ocelotl @pavolloffay @tedsuo

[pavolloffay]

Is there a way to minimize throwing exceptions when no equivalent OTEL API is available? This effectively means that the shim is not fully OpenTracing compatible and might require instrumentation changes.

[carlosalberto]

Is there a way to minimize throwing exceptions when no equivalent OTEL API is available? This effectively means that the shim is not fully OpenTracing compatible and might require instrumentation changes.

That's a good point. Error reporting guidelines have changed ever since we created the Shim for Java, which means that for such operations so should not throw any error - but maybe we can log it (as a relatively severe warning), in order to allow users to see that.

@yurishkuro @tedsuo any feedback on this?

[carlosalberto]

@pavolloffay Updated it to include the timestamp mentions (had totally overseen that, thanks) and for unrecognized tag types, we don't generate any error, but try to log a warning instead.

[carlosalberto]

Updated. Please review @pavolloffay @bogdandrutu @yurishkuro

[andrewhsu]

Curious to know if collector receivers should be in scope for the compatibility requirements?

[yurishkuro]

I am not sure I understand this. Can this even be supported transparently? In OT the SetBaggage is a void function. If it were to be translated into OTel CorrelationContext change, would it not result in the creation of a new instance of the Context object (which may or may not need to be made "current")?

[yurishkuro]

"must be implemented" is unclear, did you mean "must embed/encapsulate ..."?

[carlosalberto]

Hey @yurishkuro

Thanks for the feedback, applied. I'm still not sure we need (nor want) to create a new Context every time SetBaggageItem() is called (which, if I understood correctly, would mean that Context itself will act as the container of both a Span and a CorrelationContext). Would you elaborate?

In general, such approach does not quite appeal to me (at this moment), as Context may 'inherit' unrelated values (values are absorbed from the current parent Context at write time). In any case, we probably need a follow up for this, as we still need to figure out what to do when the current OT Span's baggage is updated (which would effectively means updating the Context).

Such case is tricky as you could have:

with setCurrentSpan(opentracingSpan):
  with updateContextWithUnrelatedItems():
     // opentracingSpan is still active, but the current Context changed,
    // so we need to handle the updated CorrelationContext properly.
    GlobalTracer.getTracer().activeSpan().setBaggageItem()

[yurishkuro]

@carlosalberto I was looking at it with this in mind:

• Context must be immutable
• CorrelationContext must be immutable (because otherwise child span's baggage may affect parent span's baggage)

Given these constraints, the only way to update baggage in OTel is to create a new Context with a new CorrContext inside, regardless of whether we're doing it via OTel API directly or via OT-shim. But OT-shim does not return anything from SetBaggage calls. That might still work with thread-local based context managers, but not with explicit contexts (e.g. Go).

[tedsuo]

Some writing suggestions for clarity:

1. Please indicate which objects/methods/interfaces are OTel vs OT. It can get confusing to tell what is what, since the terms can be so similar.
2. Please clarify that the intention is to allow OpenTracing instrumentation to be recorded by the OpenTelemetry SDK. If the intention is to allow any OTel implementation to receive OT calls, and there should be no dependency on the OTel SDK – AKA this is purely an API to API shim – please mention that.
3. Please clarify that this shim does not also allow older OT tracer to consume newer OTel API calls, it only allows the new OTel implementations to consume the deprecated OT API.

[tedsuo]

Please mention how the individual OT SpanContext methods match to the underlying OTel methods.

[tedsuo]

One more point: This doc almost covers all the OT method calls. Some method calls are described in their own headers, some calls are mixed in with the object descriptions, and some calls are missing.

Normalizing the format of this document to something like the following could help comprehension.

NewOpenTracingShim

• Describes the constructor in its own section, before describing the OT objects (Right now it is mixed in with the OT Tracer description).
• Mentions that this constructor is in the shim, not OTel or OT.
• Ensures that objects like CorrelationContextManagers, Propagators, etc are attributed to OT and OTel, respectively.
• If the OT Tracer behavior is dependent on the properties of the objects passed to this constructor, please describe how in more detail. (How do context managers and propagators change behavior?)

OpenTracing [ObjectName]

• Contains a description about how the OT object is represented by the underlying OTel objects. Any mention of an OTel object should be preceded with the word "OTel" so users understand which objects are which.
• Describes any memory or state which must live in the shim and be maintained by the shim layer.

[ObjectName].[MethodName]

• Describes the mapping between the OT method call and the underlying OTel method calls.
• Describes how the shim should manage any needed internal state.
• Mentions any gotchas and clarifications, especially if there is a difference in functionality between similar concepts in both APIs.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[carlosalberto]

Hey @tedsuo @yurishkuro I have a pair of doubts on how to tackle the Span / Baggage union as defined in OT, mostly because Context is immutable, and the calls to OT's setBaggageItem() would require Context be modifiable (this is only for languages having Global Context, so this rules out Golang).

(Note: Both of this -inusual- cases become tricky when the user is allowed to mix OT with OTel in the same code base. But it looks supporting such case is actually very important ;) )

Baggage being overwritten by OTel

Activating io.opentracing.Span will put its related Baggage in Context, but it may be overwritten by an OTel call:

try (io.opentracing.Scope scope = tracer.activateSpan(opentracingSpan)) {
  try (io.opentelemetry.context.Scope scope2 = setCurrentBaggage(otelBaggageObj)) {
    // Will return the contents of `otelBaggageObj`.
    getCurrentBaggage().getEntryValue();

    // Should *still* return the contents of the `Baggage` obj linked to `opentracingSpan`, right?
    opentracingSpan.getBaggageItem();
  }
}

Proposal: Do keep the invariant above. OT Shim will always get the linked Baggage, even if overwritten by OTel Baggage API.

Always keeping the activated Span's Baggage in the CURRENT Context

try (io.opentracing.Scope scope = tracer.activateSpan(opentracingSpan)) {
  // Changes the `Baggage` obj linked to `opentracingSpan`. Easily doable.
  opentracingSpan.setBaggageItem();

  // PROBLEM: to reflect this at the underneath OTel Baggage API, we need to
  // create a new Scope/Context with the resulting Baggage from the previous call.
  getCurrentBaggage().getEntryValues();
}

SetBaggageItem() requires the creation of a new Scope / Context, but it becomes very tricky when to close this second, implicit Scope.

Proposal 1: Do not create new Scope / Context objects upon Span.setBaggageItem() calls. The underlying Span will still be linked to Baggage, and will be properly propagated inter-process.

Proposal 2: When SetBaggageItem() happens, create a new Scope / Context and attach it to its parent Scope, so it can be automatically closed upon then. Hackish but doable (and should work fine).

[carlosalberto]

Ping @yurishkuro @tedsuo ;)

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[tedsuo]

@carlosalberto Proposal 2 looks cleaner, and feels like it is inline with how baggage is added in OpenTelemetry (you always create a new context). I would suggest going with that.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[andrewhsu]

I suggest changing all usage of "OT" to "OpenTracing", because from top-level README.md:

Please refrain from using "OT" in order to avoid confusion with the now deprecated "OpenTracing" project.

I also suggest keeping with consistent usage of "OpenTelemetry" instead of "OTel". It'll then make it easier for the reader to pickup that the document is only talking about "OpenTracing" and "OpenTelemetry" and not a third thing.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.