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

Consistently return ScopeShim and SpanShim objects in the OT shim #242

Closed
johananl opened this issue Oct 25, 2019 · 0 comments · Fixed by #922
Closed

Consistently return ScopeShim and SpanShim objects in the OT shim #242

johananl opened this issue Oct 25, 2019 · 0 comments · Fixed by #922
Assignees
Labels
release:required-for-ga To be resolved before GA release shim OpenTracing or OpenCensus compatibility tracing

Comments

@johananl
Copy link
Contributor

johananl commented Oct 25, 2019

In its current implementation, the OT shim doesn't preserve ScopeShim and SpanShim objects after creating them. This results in situations such as the following:

  1. The instrumentation creates and activates a span using start_active_span(), which results in a ScopeShim object containing a SpanShim object.
  2. The instrumentation then obtains the currently-active span using the shim's active_span property.
  3. The ScopeShim returned by active_span isn't the same object as the one returned from start_active_span().

The reason for the behavior described above is that we only keep state in the OpenTelemetry tracer. We don't really have an OpenTracing tracer or scope manager which keep track of spans. When the user queries the shim's API for the currently-active span for example, we construct a new object from the existing OpenTelemetry object. So the returned object contains the same OpenTelemetry span, but the wrapper objects themselves aren't the same OpenTracing objects as the ones returned to the user during span creation and activation.

There doesn't seem to be an easy way to address this issue. @Oberon00 has some ideas on how this can be implemented.

@property
def active(self):
span = self._tracer.unwrap().get_current_span()
if span is None:
return None
span_context = SpanContextShim(span.get_context())
wrapped_span = SpanShim(self._tracer, span_context, span)
return ScopeShim(self, span=wrapped_span)
# TODO: The returned `ScopeShim` instance here always ends the
# corresponding span, regardless of the `finish_on_close` value used
# when activating the span. This is because here we return a *new*
# `ScopeShim` rather than returning a saved instance of `ScopeShim`.
# https://github.com/open-telemetry/opentelemetry-python/pull/211/files#r335398792

# TODO: We can't check for equality of self.shim.active_span and
# scope.span because the same OpenTelemetry span is returned inside
# different SpanShim objects. A possible solution is described
# here:
# https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274

# TODO: Check equality of the spans themselves rather than
# their context once the SpanShim reconstruction problem has
# been addressed (see previous TODO).

Relevant discussions:

#211 (comment)
#161 (comment)

@Oberon00 Oberon00 added shim OpenTracing or OpenCensus compatibility tracing labels Oct 28, 2019
@ocelotl ocelotl self-assigned this Jul 11, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 17, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 17, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 18, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 18, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 20, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 20, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 20, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 20, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 20, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 21, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 22, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 22, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 22, 2020
@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Jul 22, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 31, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 31, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 31, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Jul 31, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 5, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 6, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 6, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 6, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 6, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 6, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 6, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Aug 6, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
closes open-telemetry#242

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
closes open-telemetry#242

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga To be resolved before GA release shim OpenTracing or OpenCensus compatibility tracing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants