-
Notifications
You must be signed in to change notification settings - Fork 166
add s/s action #4038
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
base: main
Are you sure you want to change the base?
add s/s action #4038
Conversation
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 66ce586 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
|
Thanks for the ping—current CI is all green and bundle size deltas are small (≈+0.8% on Rum/Rum Slim, others unchanged). Patch coverage on the new lines is ~85% with overall coverage unchanged aside from reported delta. Let me know if you want me to tackle any follow-ups. |
|
@codex pls review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review pls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review pls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8725bad56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| configuration: RumConfiguration | ||
| ) { | ||
| const customActionHistory = createValueHistory<string>({ expireDelay: ACTION_CONTEXT_TIME_OUT_DELAY }) | ||
| const activeCustomActions = new Map<string, ActiveCustomAction>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be a weakMap like in VitalCollection so it gets garbage collected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't have a weakmap with primitive types like string as key. So no, that won't be an option. This is why FeatureOperation uses an object as "key" instead of a string.
|
@codex review pls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80850cfddb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0cdbbeb to
ee43b93
Compare
|
@codex pls review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b690e5e49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function getActionLookupKey(name: string, actionKey?: string): string { | ||
| return JSON.stringify({ name, actionKey }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I'm not sure we need to care if the same action key is used with different name... do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I added it just to be safe. But agreed to remove it.
| return duration | ||
| }, | ||
| get counts() { | ||
| const { errorCount, longTaskCount, resourceCount } = eventCountsSubscription.eventCounts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to ask an agent how to try to reduce bundle size. For example here.
|
@codex can you give a review and suggest where we could save on bundle size? Is there room for improvement? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20019e376c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| HookNames.AssembleTelemetry, | ||
| ({ startTime }): DefaultTelemetryEventAttributes => ({ | ||
| action: { id: actionContexts.findActionId(startTime) as string }, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid array action.id in telemetry events
With start/stop actions, multiple concurrent custom actions are supported (via actionKey), so actionContexts.findActionId() can return a string[]. In AssembleTelemetry this value is written directly to action.id, but telemetry schemas define action.id as a single string (see packages/core/src/domain/telemetry/telemetryEvent.types.ts). During overlapping actions, any telemetry event emitted in that window will carry an array and can be rejected or dropped by downstream validation. Consider selecting one id (e.g., most recent) or omitting the action context when multiple ids are active.
Useful? React with 👍 / 👎.
| const customActions = trackCustomActions(lifeCycle, actionTracker, (action) => { | ||
| lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, processAction(action)) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gate custom action tracking to avoid bundle/runtime overhead
Custom action tracking is instantiated unconditionally, even when START_STOP_ACTION is disabled or never used. This means the extra tracker code and lifecycle subscriptions are included and executed for all builds, increasing bundle size and baseline work. If this feature is meant to be experimental/opt‑in, consider lazily initializing trackCustomActions (and its shared tracker) behind the feature flag or on first startAction call to keep the default bundle smaller.
Useful? React with 👍 / 👎.
Motivation
Discussed in this RFC
Tldr:
addEventdoes not cover all the cases needed for complete action collection in frameworks like Flutter.This PR adds Start and Stop action.
Changes
Added telemetry event type to test locally (For CI to pass, need to add in RumEventsFormat PR)
Added FF: START_STOP_ACTION
Expanded ActionType
Created expanded
startActionCollection, createdstartCustomActionInternalandstopCustomActionInternalTest instructions
I used this HTML file to test locally.
Should be able to start an action, generate events and stop the action. When in RUM explorer Attributes for each Event should have the Action.ID.
Example action with error, long_task and resource.
Example in Flutter web.
Checklist