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.

Sign up for GitHub

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

Jump to bottom

Fix invalid timings in span events #4486

Merged
merged 16 commits into from
Apr 3, 2024
Merged

Fix invalid timings in span events #4486

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0efb5da
fix: use reference value to avoid invalid timings in span
Abinet18 Feb 15, 2024
03e84e3
add comment, add to changelog
Abinet18 Feb 22, 2024
0e90f68
Merge branch 'main' into fix-invalid-timings-in-spans
Abinet18 Feb 29, 2024
18fa4c0
donot report invalid timing in span
Abinet18 Mar 6, 2024
f7880ce
lint fix
Abinet18 Mar 6, 2024
4e87873
Merge branch 'main' into fix-invalid-timings-in-spans
Abinet18 Mar 6, 2024
6dfef84
fix failing test
Abinet18 Mar 6, 2024
24c7f6c
fix failing test 2
Abinet18 Mar 6, 2024
c773b8f
Add tests
Abinet18 Mar 13, 2024
b9bf4ea
Merge branch 'main' into fix-invalid-timings-in-spans
Abinet18 Mar 13, 2024
441a301
add removed test
Abinet18 Mar 13, 2024
4f98c14
suggested changes
Abinet18 Mar 20, 2024
a3ccf1e
Merge branch 'main' into fix-invalid-timings-in-spans
Abinet18 Mar 20, 2024
6ac28d4
Merge branch 'main' into fix-invalid-timings-in-spans
Abinet18 Mar 21, 2024
cec2c3b
Merge branch 'main' into fix-invalid-timings-in-spans
Abinet18 Mar 25, 2024
98c1496
Merge branch 'main' into fix-invalid-timings-in-spans
pichlermarc Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :bug: (Bug Fix)

* fix(sdk-trace-web): fix invalid timings in span events [#4486](https://github.com/open-telemetry/opentelemetry-js/pull/4486) @Abinet18
Abinet18 marked this conversation as resolved.
Show resolved Hide resolved
* fix(sdk-metrics): handle zero bucket counts in exponential histogram merge [#4459](https://github.com/open-telemetry/opentelemetry-js/pull/4459) @mwear
* fix(sdk-metrics): ignore `NaN` value recordings in Histograms [#4455](https://github.com/open-telemetry/opentelemetry-js/pull/4455) @pichlermarc
* fixes a bug where recording `NaN` on a histogram would result in the sum of bucket count values not matching the overall count
Expand Down
12 changes: 10 additions & 2 deletions packages/opentelemetry-sdk-trace-web/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,21 @@ export function hasKey<O extends object>(
export function addSpanNetworkEvent(
span: api.Span,
performanceName: string,
entries: PerformanceEntries
entries: PerformanceEntries,
refPerfName?: string
Abinet18 marked this conversation as resolved.
Show resolved Hide resolved
): api.Span | undefined {
if (
hasKey(entries, performanceName) &&
typeof entries[performanceName] === 'number'
) {
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

// using FETCH START time in case no reference is provided
if (hasKey(entries, refName) && typeof entries[refName] === 'number') {
perfTime = Math.max(perfTime || 0, entries[refName] || 0);
}
span.addEvent(performanceName, perfTime);
return span;
}
return undefined;
Expand Down