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

✨ [RUM-6581] Add an init parameter to chose feature flags event collection #3283

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RomanGaignault
Copy link
Contributor

@RomanGaignault RomanGaignault commented Jan 14, 2025

Motivation

For now, feature flags are only included in views and errors. This was chosen to limit the feature flags impact on bandwidth, but now that we implemented compression, we could re-evaluate.

Changes

Instead of enabling feature flag collections on all events at once, we could provide an init parameter to allow customers to chose on which events they want to collect feature flags:
init({
collectFeatureFlagsOn: ['long_task', 'error', 'vital']
})

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@RomanGaignault RomanGaignault requested a review from a team as a code owner January 14, 2025 12:42
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.68%. Comparing base (1a38806) to head (54e65ab).

Files with missing lines Patch % Lines
packages/rum-core/src/boot/startRum.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3283      +/-   ##
==========================================
+ Coverage   93.67%   93.68%   +0.01%     
==========================================
  Files         288      289       +1     
  Lines        7600     7618      +18     
  Branches     1730     1735       +5     
==========================================
+ Hits         7119     7137      +18     
  Misses        481      481              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 144.58 KiB 145.50 KiB 937 B +0.63%
Logs 51.09 KiB 51.09 KiB 0 B 0.00%
Rum Slim 103.42 KiB 104.32 KiB 917 B +0.87%
Worker 24.50 KiB 24.50 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base Average Cpu Time (ms) Local Average Cpu Time (ms) 𝚫
addglobalcontext 0.003 0.002 -0.000
addaction 0.042 0.053 0.011
addtiming 0.001 0.001 0.000
adderror 0.050 0.095 0.044
startstopsessionreplayrecording 0.010 0.013 0.003
startview 0.463 0.490 0.027
logmessage 0.031 0.032 0.001
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 27.68 KiB 28.57 KiB 911 B
addaction 56.92 KiB 59.05 KiB 2.13 KiB
addtiming 26.42 KiB 27.46 KiB 1.04 KiB
adderror 60.65 KiB 61.36 KiB 729 B
startstopsessionreplayrecording 25.01 KiB 26.71 KiB 1.71 KiB
startview 411.17 KiB 416.25 KiB 5.08 KiB
logmessage 60.54 KiB 61.16 KiB 627 B

🔗 RealWorld

Comment on lines +13 to +18
if (trackFeatureFlagsForEvents.includes(eventType)) {
const featureFlagContext = featureFlagContexts.findFeatureFlagEvaluations(eventStartTime)
if (featureFlagContext && !isEmptyObject(featureFlagContext)) {
rawRumEvent.feature_flags = featureFlagContext
}
}
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: refactor your PR by ‏moving this in assembly.ts

Copy link
Collaborator

Choose a reason for hiding this comment

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

@RomanGaignault, just for context, the assembly is where we set attributes that apply on multiple event types.
For instance:

  • If an attribute applies on only on resources => resourceCollection.ts
  • If an attribute applies on resource, error etc. => assembly.ts

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably look at what we do here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I will update that ! Thanks for context 👍

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