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

[RUMF-636] initial document trace id #492

Merged
merged 10 commits into from Aug 24, 2020
Merged

[RUMF-636] initial document trace id #492

merged 10 commits into from Aug 24, 2020

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

Retrieve the trace id generated by APM for the initial document load and associate it with the generated document resource event.

Changes

  • Adjust TS typings so we can set a traceId to the performance resource timing object
  • Look for data set by the APM into the document, and parse it to extract a trace id and trace time
  • Use this data if the trace time is recent enough to define the trace id on the initial document resource event

Testing

I have gone over the contributing documentation.

@BenoitZugmeyer
[RUM] handle isTrackedWithResource before creating the event
da564fb 
* This simplifies a bit the performance collection
* This makes it closer to how trackRequests works
* This fixes the trackPageActivities computation (it should not be
impacted by whether resources are collected or not)
@BenoitZugmeyer
[RUM] don't use dom.d.ts performance entry types
87c68d7 
Two reasons:

* custom types are easier to fake for manually created data and mock
during tests. It requires less cast. Also, we already had "fake" types
accross the codebase.
* it will be used to add optional properties like "traceId"
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/initial-document-trace-id branch from 570b0ea to 37db725 Compare August 14, 2020 16:32
@BenoitZugmeyer BenoitZugmeyer changed the title Benoit/initial document trace [RUMF-636] initial document trace id Aug 14, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2020 •
edited

Codecov Report

Merging #492 into master will increase coverage by 0.05%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   87.69%   87.74%   +0.05%     
==========================================
  Files          34       35       +1     
  Lines        2113     2155      +42     
  Branches      417      433      +16     
==========================================
+ Hits         1853     1891      +38     
- Misses        260      264       +4
Impacted Files Coverage Δ
packages/rum/src/lifeCycle.ts 100.00% <ø> (ø)
packages/rum/src/viewCollection.ts 96.26% <ø> (ø)
packages/rum/src/rum.ts 86.82% <71.42%> (-4.02%) ⬇️
packages/rum/src/performanceCollection.ts 61.66% <72.00%> (-3.91%) ⬇️
packages/core/src/cookie.ts 91.66% <100.00%> (-0.23%) ⬇️
packages/core/src/utils.ts 87.20% <100.00%> (+0.22%) ⬆️
packages/rum/src/getDocumentTraceId.ts 100.00% <100.00%> (ø)
packages/rum/src/matchRequestTiming.ts 94.11% <100.00%> (+0.36%) ⬆️
packages/rum/src/requestCollection.ts 96.87% <100.00%> (ø)
packages/rum/src/resourceUtils.ts 97.46% <100.00%> (ø)
... and 6 more

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 48bb4a4...702ec9c. Read the comment docs.

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/initial-document-trace-id branch 3 times, most recently from f38ee77 to 46823e8 Compare August 17, 2020 13:44
@BenoitZugmeyer BenoitZugmeyer marked this pull request as ready for review August 17, 2020 13:57
@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner August 17, 2020 13:57
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/initial-document-trace-id branch from 46823e8 to 949f4f7 Compare August 17, 2020 14:44
mquentin
mquentin approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@mquentin mquentin left a comment

Choose a reason for hiding this comment

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

Looks good to me once your are done with the e2e tests

mquentin
mquentin approved these changes Aug 21, 2020
View reviewed changes
Copy link
Contributor

@mquentin mquentin left a comment

Choose a reason for hiding this comment

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

lgtm !

bcaudan
bcaudan reviewed Aug 24, 2020
View reviewed changes
packages/rum/src/rum.ts Outdated Show resolved Hide resolved
packages/rum/src/performanceCollection.ts Show resolved Hide resolved
packages/rum/src/performanceCollection.ts Show resolved Hide resolved
packages/rum/src/resourceUtils.ts Outdated Show resolved Hide resolved
packages/core/src/utils.ts Show resolved Hide resolved
packages/rum/src/getAPMDocumentData.ts Outdated Show resolved Hide resolved
packages/rum/src/performanceCollection.ts Outdated Show resolved Hide resolved
bcaudan
bcaudan approved these changes Aug 24, 2020
View reviewed changes
Copy link
Collaborator

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

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

Great 👍

@BenoitZugmeyer BenoitZugmeyer merged commit de0cae4 into master Aug 24, 2020
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/initial-document-trace-id branch August 24, 2020 15:58
@ryush00 ryush00 mentioned this pull request Aug 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcaudan bcaudan bcaudan approved these changes

@mquentin mquentin mquentin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@BenoitZugmeyer @codecov-commenter @bcaudan @mquentin