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-1288] Collect viewport size #1584

Merged
merged 8 commits into from
Jun 16, 2022

Conversation

amortemousque
Copy link
Contributor

@amortemousque amortemousque commented Jun 8, 2022

Motivation

When looking at clickmaps users need to be able to filter events by viewport size.
I also took to chance to clean a bit RUM domain folder by moving context files in a dedicated folder.

Changes

  • Move context files in a dedicated folder
  • Collocate context types with their modules
  • Add displayContext

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@amortemousque amortemousque force-pushed the aymeric/collect-viewport-size branch 2 times, most recently from 7b1335a to 824f9fd Compare June 9, 2022 10:21
@amortemousque amortemousque force-pushed the aymeric/collect-viewport-size branch from 824f9fd to 3adb44b Compare June 9, 2022 12:35
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #1584 (be14888) into main (8577823) will increase coverage by 0.09%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##             main    #1584      +/-   ##
==========================================
+ Coverage   90.74%   90.84%   +0.09%     
==========================================
  Files         121      123       +2     
  Lines        4540     4554      +14     
  Branches     1018     1021       +3     
==========================================
+ Hits         4120     4137      +17     
+ Misses        420      417       -3     
Impacted Files Coverage Δ
packages/rum-core/src/boot/rumPublicApi.ts 94.79% <ø> (ø)
packages/rum-core/src/boot/startRum.ts 26.31% <ø> (ø)
packages/rum-core/src/domain/assembly.ts 100.00% <ø> (ø)
...ages/rum-core/src/domain/contexts/ciTestContext.ts 100.00% <ø> (ø)
...rum-core/src/domain/contexts/foregroundContexts.ts 90.76% <ø> (ø)
...es/rum-core/src/domain/contexts/internalContext.ts 100.00% <ø> (ø)
.../rum-core/src/domain/contexts/syntheticsContext.ts 100.00% <ø> (ø)
...ckages/rum-core/src/domain/contexts/urlContexts.ts 100.00% <ø> (ø)
...kages/rum-core/src/domain/contexts/viewContexts.ts 100.00% <ø> (ø)
...ain/rumEventsCollection/action/actionCollection.ts 95.45% <ø> (ø)
... and 8 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@amortemousque amortemousque marked this pull request as ready for review June 10, 2022 09:39
@amortemousque amortemousque requested a review from a team as a code owner June 10, 2022 09:39
@@ -121,6 +122,7 @@ export function startRumAssembly(
},
synthetics: syntheticsContext,
ci_test: ciTestContext,
display: getDisplayContext(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting the display context at the assembly stage is the more straightforward way to implement it.
However, for pre-init calls, it means we won't get the display info of the time the event happens, but at the init time. I think it's good enough though

Comment on lines 8 to 9
width: window.innerWidth,
height: window.innerHeight,
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏Apparently getting the window dimension might force the browser to recompute the layout (cf list here). Maybe listen for resize event and cache its value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, good. Do you think we should do the same here

Copy link
Member

Choose a reason for hiding this comment

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

that's already what we do :) (but also we throttle the listener)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice!

@amortemousque amortemousque requested a review from a team as a code owner June 13, 2022 10:21
Mutualize recorder and rum code to avoid reflow as much a possible
@amortemousque amortemousque force-pushed the aymeric/collect-viewport-size branch from 4dc6529 to 40f03ee Compare June 13, 2022 12:38
@amortemousque amortemousque force-pushed the aymeric/collect-viewport-size branch from 0f3bd97 to 2e4144d Compare June 15, 2022 08:15
@amortemousque amortemousque merged commit 7ab2389 into main Jun 16, 2022
@amortemousque amortemousque deleted the aymeric/collect-viewport-size branch June 16, 2022 12:44
@abramjstamper
Copy link

I've checked the documentation here. What configuration of @datadog/browser-rum is required to enable getting the viewport size sent to DataDog? I was going to implement a crudely made custom metric and send the data myself, but would rather use a natively-available Data Dog feature. Thanks

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