[RUM-16386] Add RemoteConfigurationProvider, typed errors, and computed var remoteConfiguration#2980
Conversation
This comment has been minimized.
This comment has been minimized.
|
@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: e89baed98b
ℹ️ 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 remoteConfigurationEndpoint: URL { | ||
| // swiftlint:disable:next force_unwrapping | ||
| URL(string: "https://sdk-configuration.\(host)/")! | ||
| URL(string: "https://cdn.datadoghq.com/rum/")! |
There was a problem hiding this comment.
When remoteConfigurationID is used with .us1_fed or .us2_fed, this now builds the fetch URL on the commercial cdn.datadoghq.com host instead of a ddog-gov.com host as the previous site-specific implementation did. That bypasses the selected FedRAMP-compatible site for remote configuration requests and also violates the root AGENTS.md rule not to change networking endpoints without explicit approval, so gov-site customers can be routed to the wrong domain.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44ec4ce954
ℹ️ 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".
| .appendingPathComponent("abc-123") | ||
| .appendingPathExtension("json") | ||
| XCTAssertEqual(url.absoluteString, "https://sdk-configuration.browser-intake-datadoghq.com/v1/abc-123.json") | ||
| XCTAssertEqual(url.absoluteString, "https://cdn.datadoghq.com/rum/v1/configurations/abc-123") |
There was a problem hiding this comment.
This assertion now expects https://cdn.datadoghq.com/rum/..., but DatadogSite.us1.remoteConfigurationEndpoint in this same commit still returns https://sdk-configuration.browser-intake-datadoghq.com/, so appending these components produces https://sdk-configuration.browser-intake-datadoghq.com/v1/configurations/abc-123 instead. As written, the endpoint test fails immediately for US1 (and the same mismatch affects the nearby encoding tests), breaking CI even before validating the synchronizer behavior.
Useful? React with 👍 / 👎.
| import Foundation | ||
|
|
||
| /// Provides the last successfully fetched remote configuration. | ||
| public protocol RemoteConfigurationProvider { |
There was a problem hiding this comment.
This adds a new public protocol in DatadogInternal (and DatadogCoreProtocol now publicly inherits it), but the repo-level AGENTS.md says “Do NOT introduce new public API without RFC review.” Since this provider is only used as an internal/test injection seam and the API surface/ObjC bridge are not updated in this commit, it leaks a stable SDK API that would need review and compatibility guarantees; make it SPI/internal or route it through the public API process before exposing it.
Useful? React with 👍 / 👎.
…ed remoteConfiguration
saraSr5
force-pushed
the
saraSr5/RUM-improvements/remote-config
branch
from
46e4ba5 to
4dad04f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
maxep
left a comment
maxep
left a comment
There was a problem hiding this comment.
Looks good, I have left few feedback 👍
|
|
||
| /// Owns the remote configuration cache and fetch lifecycle. | ||
| /// `nil` when `remoteConfiguration` was not set at init. | ||
| internal let synchronizer: RemoteConfigurationSynchronizer? |
There was a problem hiding this comment.
Lets remove the RemoteConfigurationSynchronizer :)
| func send(request: URLRequest, delegate: URLSessionTaskDelegate?, completion: @escaping (Result<HTTPURLResponse, Error>) -> Void) { | ||
| completion(.failure(URLSessionTransportInconsistencyException())) | ||
| } | ||
|
|
||
| func fetch(request: URLRequest, completion: @escaping (Result<(HTTPURLResponse, Data), Error>) -> Void) { | ||
| completion(result) | ||
| } | ||
| } |
There was a problem hiding this comment.
I missed this in previous PR, but I think we should rely on a single send method in the protocol HTTPClient with the following signature:
func send(request: URLRequest, delegate: URLSessionTaskDelegate?, completion: @escaping (Result<(HTTPURLResponse, Data?), Error>) -> VoidThis clarify the usage and avoid having to provide a default implementation in the protocol extension.
There was a problem hiding this comment.
This protocol belongs to theDatadogCore module as it doesn't need to be exposed to feature modules.
| } | ||
|
|
||
| /// Default implementation that returns no remote configuration. | ||
| public struct DefaultRemoteConfigurationProvider: RemoteConfigurationProvider { |
There was a problem hiding this comment.
| public struct DefaultRemoteConfigurationProvider: RemoteConfigurationProvider { | |
| public struct NOPRemoteConfigurationProvider: RemoteConfigurationProvider { |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
| let error = RemoteConfigurationError.httpError(http.statusCode) | ||
| completionHandler(.failure(error)) |
There was a problem hiding this comment.
nit/
| let error = RemoteConfigurationError.httpError(http.statusCode) | |
| completionHandler(.failure(error)) | |
| completionHandler(.failure(httpError(http.statusCode))) |
| do { | ||
| data = try directory.file(named: "\(id).json").read() | ||
| } catch { | ||
| return .failure(.diskError) |
There was a problem hiding this comment.
suggestion/ Lets report a failure to telemetry
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d70251bc51
ℹ️ 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".
|
|
||
| // TODO RUM-16387: Validate the schema before saving | ||
| let remoteConfiguration: RemoteConfiguration | ||
| switch Self.decode(data) { |
There was a problem hiding this comment.
When the CDN returns a syntactically valid remote-config JSON that this SDK cannot fully decode yet (for example a new enum value in a server-controlled field), this eager decode returns before the disk write, so the raw document is never cached. That regresses the documented remoteConfigurationID behavior of fetching raw JSON for subsequent launches and makes older SDKs stop updating whenever the remote schema evolves; persist the non-empty body first and decode only when exposing the typed value.
Useful? React with 👍 / 👎.
|
|
||
| /// The last successfully fetched remote configuration, if any. | ||
| var remoteConfiguration: RemoteConfiguration? { | ||
| remoteConfigurationProvider?.remoteConfiguration |
There was a problem hiding this comment.
With remoteConfigurationID set, this computed property reads the provider's live cache, which sync() mutates after initialization. If the CDN request finishes before a feature is enabled (for example an app delays RUM.enable or a test uses a local protocol), a first-ever launch can receive a freshly fetched configuration instead of nil, despite the core protocol requiring the document cached at initialization time; this makes remote configuration application depend on network timing, so snapshot the initial cache separately from the async refresh.
Useful? React with 👍 / 👎.
| remoteConfigurationProvider.sync { result in | ||
| if case .failure(let error) = result { | ||
| self.telemetry.error("[RemoteConfig] Sync failed", error: error) |
There was a problem hiding this comment.
When remoteConfigurationID is enabled and the CDN request is still in flight, this closure retains DatadogCore until the request completes; the previous path used [weak self]. If an app stops or deinitializes the SDK on a poor network before the request returns, the core and provider stay alive past teardown, so use a weak capture for this telemetry-only callback.
Useful? React with 👍 / 👎.
2 participants
What and why?
Introduces a set of code quality improvements to the remote configuration layer, following review feedback:
How?
Review checklist
make api-surfacewhen adding new APIs