[chain-pr 5/10] DatadogLogs: typed-bus migration - #2931
Conversation
This comment has been minimized.
This comment has been minimized.
ed91047 to
2469c47
Compare
21314b1 to
ec7dc67
Compare
2469c47 to
588136c
Compare
ec7dc67 to
ba7bf22
Compare
588136c to
c482945
Compare
ba7bf22 to
a00efe9
Compare
c482945 to
c2710e2
Compare
a00efe9 to
935cf5e
Compare
12483fe to
39dd976
Compare
935cf5e to
1bfa0e3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd5e9e2e56
ℹ️ 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".
arroz
left a comment
There was a problem hiding this comment.
Looks good! Left a comment regarding memory handling, feel free to dismiss if it's safe.
CrashContextCoreProvider consumes LogEventAttributes as a legacy FeatureMessageReceiver, so routing Logs.addAttribute/removeAttribute through the typed bus dropped global log attributes from crash reports. Revert sendAttributesChanged to the legacy bus until CrashReporting migrates, and add an integration test covering the crash-context path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f8da76055
ℹ️ 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".
| try core.register(feature: feature) | ||
|
|
||
| // Subscribe typed-bus receivers: | ||
| core.messageBus.subscribe(receiver: feature.logMessageReceiver) |
There was a problem hiding this comment.
Avoid accumulating log receivers on re-enable
When Logs.enable(in:) is called more than once for the same core, each call creates a new LogMessageReceiver instance and subscribes it here. The typed bus de-duplicates only the same object identity, whereas the old legacy receiver was replaced under the single "logging" feature key, so subsequent TracingWithLoggingIntegration / span.log LogMessages are delivered to every accumulated receiver and written multiple times.
Useful? React with 👍 / 👎.
What and why?
With
CoreMessageBusimplementing the typedMessageBusprotocol (PR #2929),DatadogLogscan retire itsFeatureMessageReceiverswitch and adopt the typed bus throughout.LogMessageReceiverandWebViewLogReceiverpreviously implementedFeatureMessageReceiverwith a runtime guard on the.payloadcase and a cast to the expected type. They now conform toBusMessageReceiverdirectly, receiving their concrete message type with no envelope or cast.RemoteLoggerpreviously calledfeatureScope.send(message: .payload(RUMErrorMessage(...)))to forward log errors to RUM. It now holds aMessageBusreference and callsmessageBus.send(message:)directly, severing the dependency onFeatureScopefor cross-feature emission.The
LogEventAttributesbroadcast triggered byLogs.updateGlobalAttribute/Logs.removeGlobalAttributealso moves fromcore.send(message: .payload(...))tocore.messageBus.send(message:).How?
MessageReceivers.swift—BusMessageReceiverconformanceBoth receivers change from
structtofinal class(required byBusMessageReceiver) and replace thereceive(message: FeatureMessage, from:) -> Boolmethod with a typed variant:LogMessageReceiverFeatureMessageReceiver— guards on.payload(log as LogMessage)BusMessageReceiver<LogMessage>— receivesLogMessagedirectlyWebViewLogReceiverFeatureMessageReceiver— guards on.webview(.log(event))BusMessageReceiver<WebViewLogMessage>— receivesWebViewLogMessagedirectlyLogsFeature.swift— retained receiver references and subscription wiringLogsFeaturenow holds strong references to the two receivers so thatLogs.enablecan subscribe them after feature creation:The old
CombinedFeatureMessageReceiverwiring in theDatadogRemoteFeatureinitialiser is replaced withNOPFeatureMessageReceiver().RemoteLogger.swift— direct bus send for RUM error forwardingRemoteLoggernow receives aMessageBusat init and stores it aslet messageBus: MessageBus. ThefeatureScopereference is no longer used for cross-feature emission.Review checklist
make api-surfacewhen adding new APIsChain overview
graph LR G1["1 — Typed MessageBus protocol and shared message types"] G2["2 — Test infrastructure for typed message bus"] G3["3 — DatadogCore: CoreMessageBus implementation and rename"] G4["4 — DatadogCrashReporting: typed-bus migration"] G5["5 — DatadogLogs: typed-bus migration"] G6["6 — DatadogTrace: typed-bus migration"] G7["7 — DatadogFlags: typed-bus migration"] G8["8 — DatadogRUM: typed-bus migration and TelemetryInterceptor merge"] G9["9 — DatadogSessionReplay and DatadogWebViewTracking: typed-bus migration"] G10["10 — Documentation updates"] G1 --> G2 G2 --> G3 G3 --> G4 G3 --> G5 G5 --> G6 G3 --> G7 G6 --> G8 G7 --> G8 G8 --> G9 G1 --> G10Merge in dependency order. PRs with no incoming edges from un-merged PRs can be reviewed in parallel.