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-909] add beforeSend context #883

Merged
merged 21 commits into from
Jun 9, 2021

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

Now that beforeSend can edit the event context, it would be useful to have access on a bit of information about the browser data that have been used to generate the event.

Changes

For each kind of RUM event, expose a domain-related 'context' as a second argument of beforeSend.

Testing

Manual, unit


I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner June 4, 2021 16:06
@BenoitZugmeyer BenoitZugmeyer marked this pull request as draft June 4, 2021 16:06
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/add-beforesend-context branch from dc46438 to 3a5e9f1 Compare June 7, 2021 08:29
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/add-beforesend-context branch from 3a5e9f1 to ca9e9d3 Compare June 7, 2021 08:32
@BenoitZugmeyer BenoitZugmeyer marked this pull request as ready for review June 7, 2021 09:20
@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2021

Codecov Report

Merging #883 (64f6991) into main (8d93832) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #883      +/-   ##
==========================================
+ Coverage   89.09%   89.13%   +0.04%     
==========================================
  Files          81       81              
  Lines        3787     3793       +6     
  Branches      845      848       +3     
==========================================
+ Hits         3374     3381       +7     
+ Misses        413      412       -1     
Impacted Files Coverage Δ
packages/core/src/browser/xhrProxy.ts 100.00% <ø> (ø)
packages/core/src/domain/configuration.ts 96.42% <ø> (ø)
packages/core/src/tools/error.ts 100.00% <ø> (ø)
packages/rum-core/src/boot/rumPublicApi.ts 94.89% <ø> (ø)
...ages/rum-core/src/browser/performanceCollection.ts 13.04% <ø> (ø)
packages/rum-core/src/domain/lifeCycle.ts 100.00% <ø> (ø)
packages/rum-core/src/domain/requestCollection.ts 87.87% <ø> (ø)
...rumEventsCollection/longTask/longTaskCollection.ts 100.00% <ø> (ø)
packages/rum-core/src/rawRumEvent.types.ts 100.00% <ø> (ø)
packages/core/src/browser/fetchProxy.ts 96.36% <100.00%> (+0.13%) ⬆️
... and 9 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 8d93832...64f6991. Read the comment docs.

Also, never expose PerformanceEntry objects directly for consistency.
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/add-beforesend-context branch from d9afc38 to d3e2519 Compare June 8, 2021 09:01
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/add-beforesend-context branch from 2a3e6ea to 5be2e79 Compare June 8, 2021 15:27
This is a middle ground between having a single type to make type
inference on an internal type (RawRumEvent), and having a structures
union making the overall type less strict.

This avoids exposing internal types, and let the user cast the domain to
particular, explicit interfaces.

Also, export those types publicly.
Implement `toJSON` for RumPerformanceLongTaskTiming, so it can be used
even during tests.
Copy link
Contributor

@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 work 🎉

@BenoitZugmeyer BenoitZugmeyer merged commit 71d257e into main Jun 9, 2021
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/add-beforesend-context branch June 9, 2021 13:03
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.

4 participants