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 invalid timings in span events #4486

Merged

Conversation

Abinet18
Copy link
Contributor

@Abinet18 Abinet18 commented Feb 15, 2024

Which problem is this PR solving?

Fix invalid timings in span events

Short description of the changes

add reference time to check if the value to be reported for the event is sane (use max of time and reference time)

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Checklist:

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

@Abinet18 Abinet18 requested a review from a team February 15, 2024 21:00
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Merging #4486 (98c1496) into main (5231aa2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4486      +/-   ##
==========================================
+ Coverage   92.80%   92.82%   +0.01%     
==========================================
  Files         328      328              
  Lines        9512     9519       +7     
  Branches     2047     2050       +3     
==========================================
+ Hits         8828     8836       +8     
+ Misses        684      683       -1     
Files Coverage Δ
packages/opentelemetry-sdk-trace-web/src/utils.ts 93.92% <100.00%> (+0.24%) ⬆️

... and 1 file with indirect coverage changes

@Abinet18
Copy link
Contributor Author

@MSNev , @martinkuba , @dyladan , can you review this ?

span.addEvent(performanceName, entries[performanceName]);
let perfTime = entries[performanceName];
const refName = refPerfName || PTN.FETCH_START;
// Use a reference time whcih is the earliest possible value so that the performance timing are earlier can be corrected to this reference time
Copy link
Contributor

Choose a reason for hiding this comment

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

This might come down to opinions but I think just dropping (ignoring) timings that are earlier than expected value is better and matches the intention of 0 values (= value not available), while this converts it to a value that makes sense but not real

eg. considering 2 easily known cases of 0-value:

  • secureConnectionStart is 0 when loading an insecure (http) resource because you don't have a secure connection there
  • most of the timings when a cross-origin request is done and there's no Timing-Allow-Origin header

This would also make it easier to avoid buggy processing of data - eg. if someone processes incoming data to get the connecting time per url by connectEnd - connectStart, they would always get 0ms to connect (which is a valid value when re-using existing connection!) for non-TAO header cross-origin requests (as this would fix them to equal fetchStart), while if it's missing there'd be an easy way to know that you shouldn't calculate a value from this span

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can discuss on which option to take, either avoiding reporting the invalid timing, or correcting it as done here. In the current condition, all timings are getting reported whether valid or invalid.

Copy link
Member

Choose a reason for hiding this comment

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

I would also lean toward conditionally adding the timing only if it's valid, and if it's not valid, don't add it. This helps prevent skewing of results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t2t2 , @JamieDanielson I have made the changes to conditionally add the timings

CHANGELOG.md Outdated Show resolved Hide resolved
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!

@pichlermarc pichlermarc merged commit e01f493 into open-telemetry:main Apr 3, 2024
20 checks passed
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Apr 24, 2024
The change in open-telemetry/opentelemetry-js#4486
means that a addSpanNetworkEvent() in  v1.24.0 and
later might get dropped -- if its time value is before the fetchStart time.
Typically this happens if the event time value is 0.
trentm added a commit to open-telemetry/opentelemetry-js-contrib that referenced this pull request Apr 25, 2024
…nts (#2145)

The change in open-telemetry/opentelemetry-js#4486
means that a addSpanNetworkEvent() in  v1.24.0 and
later might get dropped -- if its time value is before the fetchStart time.
Typically this happens if the event time value is 0.
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
* fix: use reference value to avoid invalid timings in span

* add comment, add to changelog

* donot report invalid timing in span

* lint fix

* fix failing test

* fix failing test 2

* Add tests

* add removed test

* suggested changes

---------

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
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.

Invalid timings sent by instrumentations (xhr,fetch,resource)
5 participants