federation icon indicating copy to clipboard operation
federation copied to clipboard

Separate package name and version in the OpenTelemetry scope

Open mmanciop opened this issue 3 years ago • 6 comments

Currently the OpenTelemetry tracer created by the Apollo Gateway is creating a Scope like:

"scope": {
  "name": "@apollo/gateway/0.49.0"
}

Embedding the version in the scope name makes it harder and less efficient to match the spans in the OpenTelemetry Collector, e.g., with the transform [1] processor: instead of using a strict match to select spans from all @apollo/gateway versions, it requires the use of a regexp matcher.

For reference, this is how the scope looks like from one of the OpenTelemetry JS instrumentation packages:

"scope": {
  "name": "opentelemetry-instrumentation-express",
  "version": "0.28.0"
}

This commit changes the way the tracer is instantiated, so that the resulting scope carries the version of the @apollo/gateway package in the version field:

"scope": {
  "name": "@apollo/gateway",
  "version": "0.49.0"
}

Note: This change will make life easier down the line when earlier versions are retired. For now, it may make existing matching rules miss. Maybe an option would be to introduce this change with the next major release.

[1] https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/transformprocessor

mmanciop avatar Aug 02 '22 13:08 mmanciop

@mmanciop: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

apollo-cla avatar Aug 02 '22 13:08 apollo-cla

Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
Latest commit bed25f35a2c0f5d85e9a5264c282a22d4b3aa722

netlify[bot] avatar Aug 02 '22 13:08 netlify[bot]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

codesandbox-ci[bot] avatar Aug 02 '22 13:08 codesandbox-ci[bot]

Thanks for the change @mmanciop !

We're having a similar debate about when to release a similar change, since it may be breaking https://github.com/apollographql/federation/issues/1938. Lemme ask around a bit and see when we should release this, and possibly release this other "breaking" OpenTelemetry change at the same time.

benweatherman avatar Aug 02 '22 13:08 benweatherman

Hi @benweatherman, have you come to a decision?

mmanciop avatar Sep 12 '22 07:09 mmanciop

Hi @mmanciop. Really sorry about the lack of progress/news here. Ben had his attention go to other places since his last comments, and we've let this fall to the wayside. Now, your proposed change certainly look more than reasonable to me, but I know next to nothing about open-telemetry and our integration of it at the moment, so I'd like to get some opinions from a more knowledgeable people regarding this (and how big of a deal it is for backward compat), but said people are a bit overbooked this week.

So I guess this is me saying that this may unfortunately still take us a week or two to get to this seriously, but we're not forgetting about it. Again, sorry it's taking this long.

pcmanus avatar Sep 12 '22 18:09 pcmanus