feat: Rack semantic stability opt in #1594
Conversation
instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/instrumentation.rb
Show resolved
Hide resolved
hannahramadan
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle,
xuan-cao-swi and
a team
as code owners
There was a problem hiding this comment.
Whew! This one covers a lot of ground. Great work catching the changes needed in the other instrumentations. Thanks for taking this on.
I think the PR description may need a little tweaking for the HTTP server spans that Rack produces.
HTTP server span names SHOULD be {method} {http.route} if there is a (low-cardinality) http.route available (see below for the exact definition of the {method} placeholder).
If there is no (low-cardinality) http.route available, HTTP server span names SHOULD be {method}.
Happy to go over any of the comments if you have questions. Mostly, I was consulting the server span parts of this doc: https://opentelemetry.io/docs/specs/semconv/non-normative/http-migration/#http-server-span-attributes
instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/railtie.rb
Outdated
Show resolved
Hide resolved
| stability_opt_in = ENV.fetch('OTEL_SEMCONV_STABILITY_OPT_IN', '') | ||
| values = stability_opt_in.split(',').map(&:strip) | ||
|
|
||
| if values.include?('http/dup') | ||
| use(*OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.middleware_args_dup) | ||
| elsif values.include?('http') | ||
| use(*OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.middleware_args_stable) | ||
| else | ||
| use(*OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.middleware_args_old) | ||
| end | ||
|
|
There was a problem hiding this comment.
Since this is a test_helper, does it make sense to update the grape appraisal to use the different env vars?
There was a problem hiding this comment.
@kaylareopelle I decided it might be best to revert changes to this test suite, and instead rely on the original method call, middleware_args, which is now aliased to middleware_args_old. It seemed like a lot of testing of the same idea: that the correct middleware is installed and correct attribute names are used. We test both in Rack and Sinatra ◡̈ This PR is so heavy I'm nervous to keep adding!
instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/instrumentation.rb
Outdated
Show resolved
Hide resolved
instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/stable/event_handler.rb
Outdated
Show resolved
Hide resolved
...entation/rack/lib/opentelemetry/instrumentation/rack/middlewares/stable/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
...entation/rack/lib/opentelemetry/instrumentation/rack/middlewares/stable/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
...entation/rack/lib/opentelemetry/instrumentation/rack/middlewares/stable/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
instrumentation/rack/test/opentelemetry/instrumentation/rack/instrumentation_test.rb
Outdated
Show resolved
Hide resolved
| def middleware_args | ||
| if config.fetch(:use_rack_events, false) == true && defined?(OpenTelemetry::Instrumentation::Rack::Middlewares::EventHandler) | ||
| [::Rack::Events, [OpenTelemetry::Instrumentation::Rack::Middlewares::EventHandler.new]] | ||
| def middleware_args_old |
There was a problem hiding this comment.
Would this break existing monkey patches of the rack instrumentation, as this is not private? Is that a reasonable concern? I understand we are not 1.x here yet so it's not technically a semver question, but just trying to be mindful around touching rack instrumentation as it's probably got a rather large install base at this point.
There was a problem hiding this comment.
Fantastic call out - it probably would break some stuff! I've aliased the new and old method definitions, as well as added a test 7290822
This PR is intended to assist in the transition from the old to new HTTP semantic conventions. Per the HTTP semantic convention stability migration spec, users should be able to set the environment variable
OTEL_SEMCONV_STABILITY_OPT_INto:httpto emit stable conventions onlyhttp/dupto emit both old and the stable conventionsHTTP GETis now justGET- specThe agent is required to maintain this bridge for 6 months and may drop the environment variable in the next major version and emit only the stable HTTP and networking conventions.
This approach was approved in #1547