fix: update context to match spec #807

robertlaurin · 2021-06-09T00:49:33Z

Moves the attach and detach methods from private instance methods to public global methods
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/context.md

Related: https://github.com/open-telemetry/opentelemetry-ruby/pull/796/files#r647055849

fbogsany · 2021-06-09T20:26:18Z

api/lib/opentelemetry/context.rb

+      # Returns the previous context so that it can be restored
+      #
+      # @param [Context] context The new context
+      # @return [Context] prev The previous context


The spec refers to this as a "Token" rather than necessarily being the previous Context. Is there value in giving ourselves that flexibility or do we not need it?

Suggested change

# @return [Context] prev The previous context

# @return [Context] The previous context

I'll admit I find the returning of a Token a bit unclear in the context of the detach documentation.

This operation is intended to help making sure the correct Context is associated with the caller's current execution unit. Users can rely on it to identify a wrong call order, i.e. trying to detach a Context that is not the current instance. In this case the operation can emit a signal to warn users of the wrong call order, such as logging an error or returning an error value.

The API MUST accept the following parameters:

A Token that was returned by a previous call to attach a Context.

What purpose does this token serve if we want to keep attaching and detaching balanced. Is there a situation where you want to detach and provide a token that doesn't match the current context?

Is there a situation where you want to detach and provide a token that doesn't match the current context?

It helps double-check that attach and detach are balanced. These functions are meant to be low-level building blocks for higher level block-structured mechanisms. Passing an opaque Token around provides implementation flexibility while also being able to check for correct use of the functions, and report meaningful errors (oh yeah, we should do that 😄 ).

So, why a Token instead of the Context? If we were to use an array as the Context stack instead of chaining contexts together internally, we'd likely use the array index as the Token instead of the Context itself.

Ok so that makes it a lot clearer, I was under the impression that the token was to be used to actually retrieve a specific context instead of a means to assert that the attach/detach calls were lined up.

I think with the fact that should allow for attaching the same context multiple times it makes sense to use an array to keep track of the context stack, and to provide the array index as token to check against.

Changes based on discussion here 0c57bd6

api/lib/opentelemetry/context.rb

fbogsany · 2021-06-11T13:32:00Z

api/lib/opentelemetry/context.rb

+      def detach(token = nil)
+        OpenTelemetry.logger.warn 'Calls to detach should match corresponding calls to attach' if token && token != stack.size


Suggested change

def detach(token = nil)

OpenTelemetry.logger.warn 'Calls to detach should match corresponding calls to attach' if token && token != stack.size

def detach(token)

calls_matched = (token == stack.size)

OpenTelemetry.logger.warn 'Calls to detach should match corresponding calls to attach' unless calls_matched

Also, do you think there's value in logging the stack at this point, or at least the caller?

I was actually thinking that it might be useful to define an exception for this situation and then have the error handler manage it. That way an application owner/instrumentation author can adjust the handling of the error based on their current needs.

api/lib/opentelemetry/context.rb

fbogsany · 2021-06-11T13:40:26Z

api/lib/opentelemetry/context.rb

+      def detach(token = nil)
+        OpenTelemetry.logger.warn 'Calls to detach should match corresponding calls to attach' if token && token != stack.size
+
+        previous_context = stack.pop || ROOT


I think the || ROOT implies the calls were mismatched - we should probably incorporate that into the calls_matched check (if the stack is empty, the calls must not have been matched).

api/lib/opentelemetry/context.rb

Co-authored-by: Francis Bogsanyi <[email protected]>

In particular open-telemetry/opentelemetry-ruby#807 changed some of the APIs we were using to get the current span for a thread. To test both 0.17.0 and this new version I moved opentelemetry to appraisals.

I've been working on integrating Datadog's continuous profiler with opentelemetry traces (see DataDog/dd-trace-rb#1568). The profiler runs on a background thread, and needs to be able to access the current context for a thread, to be able to get the current span's trace id and span ids (if any active). To do so, I was originally using ```ruby thread = ... ::OpenTelemetry::Trace.current_span(thread[::OpenTelemetry::Context::KEY]) ``` Unfortunately, after open-telemetry#807, this interface was changed, and more importantly, `Context::KEY` was removed and replaced with `Context::STACK_KEY`. `STACK_KEY` was marked as `private_constant`. With 1.0.0.rc2, the only way of getting this information is by relying on private implementation details, which isn't great. Thus, I would like to ask if it'd be reasonable to add an optional `thread` parameter to `Context.current`. This would make it easy to access the needed information, and it would even be more future-proof as the outside code doesn't need to care anymore where and how the context is stored.

In particular open-telemetry/opentelemetry-ruby#807 changed some of the APIs we were using to get the current span for a thread. To test both 0.17.0 and this new version I moved opentelemetry to appraisals.

robertlaurin requested a review from fbogsany June 9, 2021 00:49

robertlaurin force-pushed the fix-context-to-spec branch 4 times, most recently from 3b76df2 to 154103d Compare June 9, 2021 18:43

robertlaurin self-assigned this Jun 9, 2021

robertlaurin force-pushed the fix-context-to-spec branch from 154103d to 3c440ed Compare June 9, 2021 18:45

robertlaurin marked this pull request as ready for review June 9, 2021 18:45

robertlaurin requested review from ahayworth, arielvalentin, dazuma, ericmustin and mwear as code owners June 9, 2021 18:45

robertlaurin mentioned this pull request Jun 9, 2021

feat: Add ActionView instrumentation via ActiveSupport::Notifications #796

Merged

fbogsany reviewed Jun 9, 2021

View reviewed changes