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

Add http.target to Django duration metric attributes #2624

Merged
merged 8 commits into from
Jul 16, 2024

Conversation

alexmojaki
Copy link
Contributor

@alexmojaki alexmojaki commented Jun 19, 2024

Description

This is similar to #2621 which is for #1457, but this PR is for Django rather than Flask.

It adds the http.target attribute to the metric named http.server.duration.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

"New feature" feels right and I've chosen it to match #2621 and its changelog update. Arguably this is a bug fix because the attribute is required by the spec, and technically it could be considered a breaking change because this will increase the number of metric data points emitted, but the attribute has low cardinality.

How Has This Been Tested?

The existing test failed until I updated it to also expect the http.target attribute.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@alexmojaki alexmojaki requested a review from a team June 19, 2024 20:49
Copy link

linux-foundation-easycla bot commented Jun 19, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@alexmojaki
Copy link
Contributor Author

alexmojaki commented Jun 20, 2024

The way I've done this may also affect other instrumentations that use _duration_attrs in util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py if those instrumentations set http.route in the right place.

I've just found that the FastAPI tests fail because of this: the metrics now have both http.route and http.target with the same value. What should be done about this? I listed some options in #1457 (comment)

Changed http.route to http.target

@alexmojaki
Copy link
Contributor Author

alexmojaki commented Jun 20, 2024

http.route is also appearing in the starlette metrics, which is causing those tests to fail, but that just means that this fix applies to both starlette and django.

@alexmojaki alexmojaki changed the title Add http.route to Django duration metric attributes Add http.target to Django duration metric attributes Jul 16, 2024
@alexmojaki
Copy link
Contributor Author

This should be mergeable now. Only thing I'm unsure about is the changelog entry. This doesn't need a documentation update, right?

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change @alexmojaki ! I know the semantic convention stuff was confusing haha.

@lzchen lzchen merged commit f8f58ee into open-telemetry:main Jul 16, 2024
379 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants