Skip to content

Conversation

@howardyoo
Copy link
Contributor

fix: http.response_content_length does not get populated when ignoreNetworkEvents is set to true

This PR fixes the issue #2879 where when the instrumentation is configured with ignoreNetworkEvents set as true, the two attributes: http.response_content_length and http.response_content_length_compressed are also getting omitted, due to a logic flaw that did not call the addSpanNetworkEvents when the ignoreNetworkEvents were set to true.

Regardless of whether ignoreNetworkEvents are set to true or false, the response content length should be returned to the resource fetch span, as it is not a part of network span events.

Fixes: #2879

A fix to make sure the span events creation will not affect the attribute creation (e.g. http.response_content_length).
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This is just applying a fix accepted elsewhere. 🚀

@dyladan
Copy link
Member

dyladan commented Jun 11, 2025

Please add tests

Copy link
Member

@JamieDanielson JamieDanielson 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 fixing this!
As Dan mentioned, just needs a test update. Also please run npm run lint to see the change needed to fix the failing status check. In fact you should be able to do npm run lint:fix and it will fix it for you.

@codecov
Copy link

codecov bot commented Jun 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.73%. Comparing base (f8ffeb0) to head (a87ab47).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2880      +/-   ##
==========================================
+ Coverage   89.68%   89.73%   +0.04%     
==========================================
  Files         187      187              
  Lines        9059     9059              
  Branches     1858     1858              
==========================================
+ Hits         8125     8129       +4     
+ Misses        934      930       -4     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@howardyoo
Copy link
Contributor Author

Thanks for fixing this! As Dan mentioned, just needs a test update. Also please run npm run lint to see the change needed to fix the failing status check. In fact you should be able to do npm run lint:fix and it will fix it for you.

Thanks, yes, ran lint:fix and fixed the formatting issue.

@howardyoo
Copy link
Contributor Author

Please add tests

Sure, added the test for this. Thanks.

@pichlermarc pichlermarc enabled auto-merge (squash) June 12, 2025 14:08
@pichlermarc pichlermarc merged commit d6e7fe7 into open-telemetry:main Jun 12, 2025
23 checks passed
@dyladan dyladan mentioned this pull request Jun 12, 2025
@howardyoo howardyoo deleted the howardyoo/2879 branch June 12, 2025 21:00
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.

http.response_content_length does not get populated when ignoreNetworkEvents is set to true

6 participants