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.

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 OpenTracing compatibility section. #1101

Merged

Conversation

carlosalberto
Copy link
Contributor

Fixes #114

This is a second attempt after #768 was closed ;) It applies all the feedback, leaving the so-called back-propagation of Baggage out, given its complexity in the implementation. It can be added later if/as needed:

try (io.opentracing.Scope scope = openTracingTracer.activateSpan(span)) {
  // New baggage item *is properly* updated in Span/Baggage, but the current Context is
  // not updated as of now.
  span.setBaggageItem("foo", "bar");
}

cc @pavolloffay @tedsuo @yurishkuro

@carlosalberto carlosalberto added spec:trace Related to the specification/trace directory area:miscellaneous For issues that don't match any other area label priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs labels Oct 16, 2020
@carlosalberto carlosalberto requested review from a team October 16, 2020 15:35
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

I'm OK with the document in current form, even though I have a few questions and suggested improvements - like semantical convention mapping

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Do we have a functional prototype of this design?

specification/compatibility/opentracing.md Outdated Show resolved Hide resolved
specification/compatibility/opentracing.md Outdated Show resolved Hide resolved
specification/compatibility/opentracing.md Outdated Show resolved Hide resolved
specification/compatibility/opentracing.md Outdated Show resolved Hide resolved
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with some minor suggestions.

@carlosalberto
Copy link
Contributor Author

carlosalberto commented Mar 9, 2021

Hey @yurishkuro

I finally managed to update the document as suggested in the previous review. A few things to summarize the state:

  • Overall re-organized the document following your recommendations.
  • Filled an issue for the "execution unit" term.
  • Allowed the Tracer Shim to get explicit, separate propagators for OT TextMap and HttpHeaders formats. Binary is not mentioned at all for now, as we don't quite define it anywhere.
  • Added code snippets for non-obvious operations - although went with Java, as I also wanted to describe ScopeManager, which doesn't exist in Go ;)
  • Will work on the Log(Exception e) -> RecordException(Exception e) translation in a follow up, in order to facilitate merging this PR.
  • Putting a Span Shim object in the Context was prototyped without problems: Store SpanShim/Baggage in the Context. opentelemetry-java#2982

The final important item to be discussed is your comment:

Specifically, in OT the span.setBaggage operation is change-in-place and affects what baggage can be seen by different threads (and, therefore, has an inherent race condition, due to OT design). In contrast, OTEL stores baggage in the Context and removes this race condition by design, but it is not clear to be how the shim deals with it since the Context is never mentioned. If we carry the Baggage object directly in the SpanContext shim, then it can reproduce the same OT behavior.

To which I had answered:

In Java and C# Baggage exists in its own object, which basically means we can leave Context entirely out (at least for those languages)... I'm also trying to keep the invariant of an actual SpanContext being entirely replaced (by calling Span.setBaggageItem())

Let me know ;)

@github-actions github-actions bot removed the Stale label Mar 9, 2021
@carlosalberto
Copy link
Contributor Author

carlosalberto commented Mar 17, 2021

Merging as we have enough approvals, and have filled the following issues as follow ups:

#1511 Execution Unit clarification
#1509 Consumption of non-supported types for SetAttribute()
#1552 Error logging translation
#1553 Bagage storage location

@yurishkuro please feel free to open any other follow up, I will be happy to address it.

I think now we need to go out and validate with Go/Python to finish validating (or correcting) the current approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:miscellaneous For issues that don't match any other area label priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a section on compatibility with OpenTracing to Compatibility doc
7 participants