Wire timeseries collection into RUM session and configuration - #2885
Conversation
barboraplasovska
changed the title
0923e06 to
c6e2b36
Compare
eb87479 to
d8e6192
Compare
c6e2b36 to
e347195
Compare
This comment has been minimized.
This comment has been minimized.
a72e13f to
a91cf87
Compare
ddb5d9b to
4344511
Compare
a91cf87 to
b0560d7
Compare
9ee6b2c to
53314ef
Compare
c9262a2 to
3f0604b
Compare
4a70e4a to
26291ad
Compare
3f0604b to
7815d4d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dec497f6d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| public var enableTimeseries: Bool | ||
|
|
||
| /// The number of samples collected before a timeseries batch is flushed. | ||
| /// | ||
| /// Default: `30`. | ||
| public var timeseriesBatchSize: Int |
There was a problem hiding this comment.
This adds the public Swift-only knobs that activate and tune the feature, but objc_RUMConfiguration in DatadogRUM/Sources/RUM+objc.swift has no forwarding properties for enableTimeseries or timeseriesBatchSize. Objective-C apps can only configure RUM through DDRUMConfiguration, so they cannot enable this new opt-in feature or set its batch size even though the SDK supports Objective-C configuration. Mirror these fields on the ObjC wrapper as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think we want to do this for now since we're still internally testing this feature.
There was a problem hiding this comment.
That makes sense for this feature branch. Can we make sure ObjC forwarding is added before this is released if the Swift configuration remains public?
There was a problem hiding this comment.
Yes I added it to my task list for later when we GA
…-footprint Rename memory_max to memory_footprint in timeseries schema and regenerate models
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fcb08027c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee5619557c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
gonzalezreal
left a comment
gonzalezreal
left a comment
There was a problem hiding this comment.
Looks good!
Could you add some tests for the new session behavior? I'd add one where a sampled-out session does not start the collector, and another one where a session created in the background pauses the collector right away.
2 participants
What and why?
Integrates the collector into the RUM stack. Adds
enableTimeseries: Bool = falsetoRUM.Configuration(all platforms including the watchOS/tvOS#elsebranch). Threads it throughRUMScopeDependenciesand creates/starts the collector inRUMSessionScopefor the session lifetime.RUMFeatureconstructs the collector when the flag is enabled. The feature is opt-in; settingenableTimeseries: trueis all that's needed to activate collection.Review checklist
make api-surfacewhen adding new APIs