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

Allow zero/negative performance timings #1769

Merged
merged 7 commits into from
Jan 20, 2021
Merged

Allow zero/negative performance timings #1769

merged 7 commits into from
Jan 20, 2021

Conversation

johnbley
Copy link
Member

Which problem is this PR solving?

  • We have noticed that some browsers (particularly Firefox and Edge) report slightly negative or 0 timings for some performance entries. This usually happens when the request is on a local network (for example, in automated tests). Currently these values are removed from the data stream and we would like them normalized and reported.

Short description of the changes

  • Allow performance timings through as long as they are numeric.

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #1769 (a0768d4) into master (6b1285c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1769      +/-   ##
==========================================
- Coverage   92.62%   92.60%   -0.02%     
==========================================
  Files         174      174              
  Lines        6049     6048       -1     
  Branches     1289     1288       -1     
==========================================
- Hits         5603     5601       -2     
- Misses        446      447       +1     
Impacted Files Coverage Δ
packages/opentelemetry-web/src/utils.ts 94.63% <ø> (-0.08%) ⬇️
...ntelemetry-web/src/enums/PerformanceTimingNames.ts 100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@@ -185,19 +185,23 @@ describe('utils', () => {
assert.strictEqual(args[1], 123);
});
});
describe('when entry has time equal to 0', () => {
describe('when entry is not numeric', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think you should still have unit test when time is 0 - this is basically what you are fixing in this PR right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call; I updated the positive test to check 0, a negative number, and a positive number.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan added the enhancement New feature or request label Jan 5, 2021
@dyladan dyladan requested a review from Flarna as a code owner January 20, 2021 16:03
@dyladan dyladan merged commit ee014c9 into open-telemetry:master Jan 20, 2021
@dyladan
Copy link
Member

dyladan commented Jan 20, 2021

Now @johnbley is an approver this meets merge requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants