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

feat(instrumentation-http): Add API for adding custom metric attributes #4108

Closed

Conversation

klippx
Copy link

@klippx klippx commented Sep 1, 2023

Which problem is this PR solving?

Add a clear and intuitive API to configure custom metric attributes that a user may inject (unrelated to span context).

Fixes #4107
Related discussion: #4088

Short description of the changes

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

There are only two metrics recorded, they should both get the custom attributes:

  • Unit test assertion: Ensure custom metric attributes are added to client http bucket
  • Unit test assertion: Ensure custom metric attributes are added to server http bucket

Checklist:

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 1, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@klippx klippx marked this pull request as ready for review September 1, 2023 09:14
@klippx klippx requested a review from a team September 1, 2023 09:14
@klippx klippx changed the title Add API for adding custom metric attributes feat(instrumentation-http) Add API for adding custom metric attributes Sep 1, 2023
@klippx klippx changed the title feat(instrumentation-http) Add API for adding custom metric attributes feat(instrumentation-http): Add API for adding custom metric attributes Sep 1, 2023
@klippx klippx force-pushed the http-add-custom-metric-attributes branch from db98bf7 to d335f6a Compare September 5, 2023 08:16
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #4108 (cc974d7) into main (73b4466) will decrease coverage by 1.78%.
The diff coverage is n/a.

❗ Current head cc974d7 differs from pull request most recent head bc47738. Consider uploading reports for the commit bc47738 to get more accurate results

@@            Coverage Diff             @@
##             main    #4108      +/-   ##
==========================================
- Coverage   92.31%   90.53%   -1.78%     
==========================================
  Files         330      159     -171     
  Lines        9417     3763    -5654     
  Branches     1993      838    -1155     
==========================================
- Hits         8693     3407    -5286     
+ Misses        724      356     -368     

see 182 files with indirect coverage changes

@klippx klippx force-pushed the http-add-custom-metric-attributes branch from d335f6a to 0525942 Compare September 21, 2023 09:31
@klippx klippx requested a review from legendecas September 21, 2023 09:38
@klippx klippx force-pushed the http-add-custom-metric-attributes branch from 039228a to a410833 Compare October 1, 2023 19:00
@klippx klippx force-pushed the http-add-custom-metric-attributes branch from a410833 to 6097474 Compare October 11, 2023 12:57
@klippx
Copy link
Author

klippx commented Oct 11, 2023

@legendecas does it look good to you now?

@klippx klippx force-pushed the http-add-custom-metric-attributes branch from 6097474 to bc47738 Compare November 7, 2023 14:22
@klippx
Copy link
Author

klippx commented Nov 7, 2023

@legendecas Hi, can we merge this please? 🙏

@klippx
Copy link
Author

klippx commented Nov 22, 2023

👻

Guess I got ghosted, lets close this.

@klippx klippx closed this Nov 22, 2023
@naorpeled
Copy link

Hey @legendecas,
hope you're doing well!
sorry for bumping this.

How can I achieve this functionality with the current implementation of the library?

@alexa-gt
Copy link

alexa-gt commented Aug 15, 2024

@klippx @legendecas what happened here?

@alexa-gt
Copy link

Never mind me. I see the changes relate to static label values only and I was looking for a dynamic one.

@GraemeF
Copy link

GraemeF commented Oct 1, 2024

I'd like to add the service name as an attribute on http metrics - seems like it isn't possible without something like this.

@klippx
Copy link
Author

klippx commented Nov 8, 2024

The marked answer has been updated to reflect how we solved this: #4088 (reply in thread)

This fixes the problem across all metrics, not only http metrics.

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.

[@opentelemetry/instrumentation-http] Configure custom metric attributes
5 participants