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

fix(instrumentation-fetch)! Remove HTTP prefix in span name. #4962

Conversation

wolfgangcodes
Copy link

Which problem is this PR solving?

Remove HTTP prefix in span name to match HTTP semantic conventions 1.18.

Similar to #3603 and #3672

Short description of the changes

Remove HTTP on span name.

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Updated existing unit tests in experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Aug 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Signed-off-by: Wolfgang Therrien <[email protected]>
@wolfgangcodes wolfgangcodes marked this pull request as ready for review August 28, 2024 21:42
@wolfgangcodes wolfgangcodes requested a review from a team August 28, 2024 21:42
@JamieDanielson JamieDanielson added spec-feature This is a request to implement a new feature which is already specified by the OTel specification spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug pkg:instrumentation-fetch labels Aug 29, 2024
@pichlermarc
Copy link
Member

The semantic conventions that we currently emit (from semconv 1.7) actually require the span name to be as it is right now (including the http prefix). However, we've dropped the prefix in other instrumentations already and they're now in an inconsistent status.

Q(@open-telemetry/javascript-maintainers): do you think should we keep the current state (inconsistent with other instrumentations) in order to keep maintenance overhead low for consumers of this package when we inevitably update this instrumentation to semconv 1.27? 🤔 Otherwise users will have to adjust how they're their telemetry twice (once with span name update, another time with update to semconv 1.27.

@pichlermarc
Copy link
Member

@wolfgangcodes I discussed this topic with the other maintainers:

Since this change would break the telemetry being emitted from the instrumentation and would make it end up in an inconsistent state (a mix between semconv versions) we're rejecting this PR and will instead drop the HTTP prefix when we update the instrumentation (span name and attributes) to semconv 1.27. Sorry for the inconvenience.

@wolfgangcodes
Copy link
Author

Thanks @pichlermarc for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:instrumentation-fetch spec-feature This is a request to implement a new feature which is already specified by the OTel specification spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants