RUM-17846: Add remote configuration sync/apply metadata to configuration telemetry - #3742
RUM-17846: Add remote configuration sync/apply metadata to configuration telemetry#3742hamorillo wants to merge 8 commits into
Conversation
|
@codex review |
![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: b8f56191e9
ℹ️ 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".
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: a684a4a | Docs | View more details | Give us feedback! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/remote-config #3742 +/- ##
=========================================================
- Coverage 73.23% 73.23% -0.00%
=========================================================
Files 1006 1007 +1
Lines 36851 36895 +44
Branches 6386 6396 +10
=========================================================
+ Hits 26986 27018 +32
- Misses 8038 8047 +9
- Partials 1827 1830 +3
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98b9cda8d6
ℹ️ 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".
| @Suppress("ThreadSafety") | ||
| syncMetadata = allowThreadDiskReads { readMetadataFromDisk() } | ||
|
|
||
| // Stamp firstApplied asynchronously — ordering is guaranteed. Only when there is a |
There was a problem hiding this comment.
what if configuration telemetry is sent before firstApplied is written? We will not send it for the session that actually applied this config. Is it possible?
There was a problem hiding this comment.
We could change syncMetadata synchronously here, but write to disk asyncronously.
There was a problem hiding this comment.
We could change syncMetadata synchronously here, but write to disk asyncronously.
We can do that, but I think it is not really necessary. Both share the same executor, so the execution will be linear with this written always executed before.
In addition, the SDK have a delay for the configuration telemetry event.
| * The outcome of a genuinely new (non-304) fetch: a fresh body along with the CDN metadata | ||
| * needed to track sync/apply telemetry for the version it belongs to. | ||
| */ | ||
| data class FetchResult( |
There was a problem hiding this comment.
Small suggestion, it might make the code more readable.
We can make this FetchResult a sealed class/interface:
sealed interface FetchResult {
data class NewResult(
body,
versionId,
lastModified,
syncTime, // goes to lastSynched later
syncId // maybe even syncId here, but up to you
): FetchResult
data class FromCache(
body,
versionId,
lastModified,
networkRequestMade, // was it 304 response or the response was returned from cache without a real network request
): FetchResult
object Error: FetchResult
}Because currently null is returned both when some error occured and when the result was obtained from cache.
There was a problem hiding this comment.
I don't think we need that distinction right now, but it's definitely a nice suggestion! Let me know if you think this is a blocker or if it could be a follow-up (happy to work on this in a different PR).
| val lastSynced: Long, | ||
| val firstApplied: Long?, | ||
| val syncId: String | ||
| ) { |
There was a problem hiding this comment.
nit:
ideally this toJson and fromJson code should be generated.
There was a problem hiding this comment.
That would be nice. This doesn't come from the schema, so I'm not sure if we've done something similar in the SDK before. I can create a ticket with low priority to review it.
…when no config is loaded
98b9cda to
a684a4a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a684a4a5d6
ℹ️ 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".
3 participants
What does this PR do?
Attaches remote configuration sync/apply metadata to the SDK's once-per-session configuration telemetry event: a
remote_configurationobject (config_id,version_id,last_modified,last_synced,first_applied,sync_id), plus the previously-unpopulatedremote_configuration_idfield.Motivation
Gives visibility into CDN-fetch-to-apply propagation delay for remote config, mirroring dd-sdk-ios#3112.
Additional Notes
RemoteConfigSyncMetadatamodel,InternalSdkCore.remoteConfigurationSyncMetadata,TelemetryConfigurationEvent.RemoteConfiguration.Review checklist (to be filled by reviewers)