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-http): add server attributes after they become available #5081

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Oct 21, 2024

Which problem is this PR solving?

While reviewing #5079 I noticed that we don't set http.route and http.response.status code in the server duration metrics, so this PR addresses a few problems:

  • We did not add http.route to the Span on "new" metrics
  • We add the http.route only when DUPLICATE emission is turned on because
    • we also didn't add it to the Span for "new" metrics
    • but we did add it for the "old" metrics and the attribute name happens to match
  • we never added the http.response.status code, because
  • it is not yet determined when we add it (before the request is handled)

NOTE: skipping changelog because this is a bug in an unreleased feature

Supersedes #5079
Related to #5026

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Added assertions to unit tests

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.26%. Comparing base (be1737f) to head (ec50dc6).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5081      +/-   ##
==========================================
- Coverage   93.27%   93.26%   -0.02%     
==========================================
  Files         317      317              
  Lines        8195     8195              
  Branches     1641     1641              
==========================================
- Hits         7644     7643       -1     
- Misses        551      552       +1     

see 1 file with indirect coverage changes

@pichlermarc pichlermarc requested a review from dyladan October 21, 2024 11:10
@pichlermarc pichlermarc marked this pull request as ready for review October 21, 2024 11:11
@pichlermarc pichlermarc requested a review from a team as a code owner October 21, 2024 11:11
@klippx
Copy link

klippx commented Oct 22, 2024

Nice! When will this be released on NPM? Just asking, because I noticed that #5079 was not released yet, only merged. Not sure what the current state is.

@pichlermarc
Copy link
Member Author

pichlermarc commented Oct 23, 2024

Nice! When will this be released on NPM? Just asking, because I noticed that #5079 was not released yet, only merged. Not sure what the current state is.

We'll release once all of these things listed in the comment here are done. 🙂

@pichlermarc pichlermarc added this pull request to the merge queue Oct 23, 2024
Merged via the queue into open-telemetry:main with commit 330172c Oct 23, 2024
21 checks passed
@pichlermarc pichlermarc deleted the fix/server-metrics branch October 23, 2024 09:22
Annosha pushed a commit to Annosha/opentelemetry-js that referenced this pull request Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants