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-1510] Warn the user when a heavy context is used #2120

Merged
merged 11 commits into from
Apr 3, 2023

Conversation

amortemousque
Copy link
Contributor

Motivation

Raise awareness about customer data impact on the user bandwidth.

Changes

  • For ssr compat:
    • Move getGlobalObject on its own file to avoid circular ref with timer.ts
    • Use getGlobalObject in setTimeout and setInterval
  • Compute customer data bytes count at API call
  • Add a console warning when 3KiB customer data reach for
    • Global context
    • User
    • Feature flag evaluation
    • Logger context (I made the call since it also uses context manager)
  • Warn once per customer data type to avoid spams

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@amortemousque amortemousque requested a review from a team as a code owner March 29, 2023 10:11
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #2120 (d031697) into main (ff79c34) will decrease coverage by 0.04%.
The diff coverage is 78.18%.

@@            Coverage Diff             @@
##             main    #2120      +/-   ##
==========================================
- Coverage   93.62%   93.58%   -0.04%     
==========================================
  Files         179      181       +2     
  Lines        5989     6003      +14     
  Branches     1344     1345       +1     
==========================================
+ Hits         5607     5618      +11     
- Misses        382      385       +3     
Impacted Files Coverage Δ
packages/core/src/tools/utils.ts 85.41% <ø> (+3.84%) ⬆️
packages/core/src/transport/eventBridge.ts 100.00% <ø> (ø)
packages/core/src/tools/getGlobalObject.ts 20.00% <20.00%> (ø)
packages/core/src/tools/contextManager.ts 94.59% <100.00%> (+0.47%) ⬆️
packages/core/src/tools/getZoneJsOriginalValue.ts 100.00% <100.00%> (ø)
...ackages/core/src/tools/heavyCustomerDataWarning.ts 100.00% <100.00%> (ø)
packages/core/src/tools/timer.ts 100.00% <100.00%> (ø)
packages/logs/src/boot/logsPublicApi.ts 98.07% <100.00%> (ø)
packages/logs/src/domain/logger.ts 94.00% <100.00%> (ø)
packages/rum-core/src/boot/rumPublicApi.ts 91.75% <100.00%> (ø)
... and 2 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@amortemousque amortemousque force-pushed the aymeric/warn-user-when-heavy-context branch from 2ee8991 to 570fcbb Compare March 29, 2023 11:52
@amortemousque amortemousque force-pushed the aymeric/warn-user-when-heavy-context branch from 570fcbb to 15b2575 Compare March 29, 2023 12:21
@@ -13,12 +13,19 @@ export const enum CustomerDataType {
LoggerContext = 'logger context',
}

let alreadyWarned: { [key: string]: boolean } = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ question: ‏FMU, for a npm setup with both logs and rum, it would mean that we would warn customers once for all SDK instances, is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it was not :)
I moved the "already warn check" to contextManager and featureFlagContext to solve it.
Let me know what you think.

@amortemousque amortemousque force-pushed the aymeric/warn-user-when-heavy-context branch from 96d3ba0 to f5a2c54 Compare March 30, 2023 09:18
@amortemousque amortemousque merged commit 198e9e5 into main Apr 3, 2023
2 checks passed
@amortemousque amortemousque deleted the aymeric/warn-user-when-heavy-context branch April 3, 2023 08:42
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.

None yet

3 participants