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

[RUMF-407] improve resource timings collection #315

Merged
merged 8 commits into from
Mar 25, 2020

Conversation

BenoitZugmeyer
Copy link
Member

With this PR, we try to improve the resources timing collection in different ways:

  • Better cross browser support:

    • If timing.duration isn't available or 0, use responseEnd - startTime instead, if available (Safari)
    • If redirectStart isn't available, use startTime instead (Firefox)
    • If redirectEnd isn't available, use fetchStart instead (Firefox)
  • Change how the timings start are computed: use difference (in nanosecond) between the request startTime and the timing start, so we can build reliable timings diagram.

    • Discard any timing lower than startTime instead of 0 since it should not happen and would provide negative timings start.
  • Report timings for all cached resources, as we've seen that we can't accurately know if a resource is cached or not (timings may not be 0 when cached)

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner March 23, 2020 17:33
@BenoitZugmeyer BenoitZugmeyer changed the title Benoit/improve resource timings collection [RUMF-407] improve resource timings collection Mar 23, 2020
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/improve-resource-timings-collection branch from 6b52e44 to 5a0ddca Compare March 23, 2020 17:54
packages/rum/src/resourceUtils.ts Show resolved Hide resolved
packages/rum/src/resourceUtils.ts Show resolved Hide resolved
Comment on lines 76 to 80
const timing = await makeXHRAndCollectEvent(`${browser.config.baseUrl}/ok`)
expect(timing).not.toBeUndefined()
expect(timing!.http.method).toEqual('GET')
expect((timing!.http as any).status_code).toEqual(200)
expectToHaveValidTimings(timing!)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const timing = await makeXHRAndCollectEvent(`${browser.config.baseUrl}/ok`)
expect(timing).not.toBeUndefined()
expect(timing!.http.method).toEqual('GET')
expect((timing!.http as any).status_code).toEqual(200)
expectToHaveValidTimings(timing!)
const timing = await makeXHRAndCollectEvent(`${browser.config.baseUrl}/ok`)!
expect(timing).not.toBeUndefined()
expect(timing.http.method).toEqual('GET')
expect((timing.http as any).status_code).toEqual(200)
expectToHaveValidTimings(timing)

and for other occurences

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. In TS 3.8, I'd like to make something like:

function expectDefined<T>(value: T | undefined): asserts value is T {
  expect(value).toBeDefined()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it worth the trouble?
IMO, it does not bring much more than the ! operator and we (or at least I 😄) could forget the existence of this helper easily since it is pretty specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll see if it can improve the situation. This is great to link the TS type to the actual JS type at runtime :) ! should be avoided if possible, in the same way as as casts.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's try

test/e2e/scenario/agents.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/agents.scenario.ts Show resolved Hide resolved
test/e2e/scenario/agents.scenario.ts Outdated Show resolved Hide resolved
test/server/fake-backend.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #315 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   85.56%   85.65%   +0.09%     
==========================================
  Files          25       25              
  Lines        1413     1429      +16     
  Branches      309      302       -7     
==========================================
+ Hits         1209     1224      +15     
- Misses        204      205       +1     
Impacted Files Coverage Δ
packages/rum/src/resourceUtils.ts 97.46% <100.00%> (+0.54%) ⬆️
packages/rum/src/rum.ts 84.14% <100.00%> (-0.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb3e15f...050f4c3. Read the comment docs.

@BenoitZugmeyer BenoitZugmeyer requested a review from bcaudan March 24, 2020 13:16
@BenoitZugmeyer BenoitZugmeyer merged commit eb56a30 into master Mar 25, 2020
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/improve-resource-timings-collection branch March 25, 2020 13:36
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.

3 participants