RUMM-2334 Use DatadogContext in V1 scopes #981
Conversation
maxep
force-pushed
the
maxep/RUMM-2334/v2-context-in-scope
branch
2 times, most recently
from
683c8c4 to
144c192
Compare
Datadog ReportBranch report: ✅ |
maxep
force-pushed
the
maxep/RUMM-2334/v2-context-in-scope
branch
2 times, most recently
from
c4c7c66 to
05775fa
Compare
| var networkConnectionInfo: NetworkConnectionInfo = .unknown | ||
| var networkConnectionInfo: NetworkConnectionInfo? |
There was a problem hiding this comment.
Making this optional to keep previous behavior.
maxep
force-pushed
the
maxep/RUMM-2334/v2-context-in-scope
branch
2 times, most recently
from
d275f69 to
b135773
Compare
maxep
force-pushed
the
maxep/RUMM-2334/v2-context-in-scope
branch
from
c058c91 to
db2072a
Compare
| queue.async { | ||
| let transformedCommand = self.transform(command: command) | ||
| core.v1.scope(for: RUMFeature.self)?.eventWriteContext { context, writer in | ||
| self.queue.sync { |
There was a problem hiding this comment.
All 3 Features now perform sync on the event write context. This is not set in stone, and I'm exploring ways of synchronising Features with the shared context.
There was a problem hiding this comment.
👍 Just submitted my review on that 🙂. Definitely, we need to explore the impact of this in high load situations. I'll think of possible solutions as well.
| queue.sync { | ||
| self.queue.sync { |
There was a problem hiding this comment.
@maxep Does lack of queue.async {} here correspond to Features context synchronisation issue we've been debating about?
There was a problem hiding this comment.
Yes! still exploring solution. It will be change in future PR.
| init(context: DatadogContext) { | ||
| self.init(device: context.device) | ||
| } | ||
|
|
||
| init(context: DatadogV1Context) { | ||
| self.init(device: context.device) | ||
| } |
There was a problem hiding this comment.
We need both because of V1 context leftover in Crash Reporting, right?
There was a problem hiding this comment.
Yes.. crash reporting and web view tracking. Both integration are still using v1 context
| attributes: .concurrent | ||
| qos: .utility |
There was a problem hiding this comment.
Given it is serial now, it no longer makes sense to do .barrier in following code, no?
func write(block: @escaping (inout DatadogContext) -> Void) {
queue.async(flags: .barrier)
}There was a problem hiding this comment.
Indeed, .barrier will have no effect on the serial queue, but I would like to keep it for now and move back to concurrency with a better threading design later.
There was a problem hiding this comment.
Got it 👌. Good to go as it is targeting feature branch 👍
What and why?
While working on Feature communication through Core Context, I realised that we cannot apply synchronisation while maintaining 2 contexts. It is time to fade away
DatadogV1Contextin favor ofDatadogContext.How?
Date Provider
The
DateProvideris no longer part of context, it is now injected as part of the configuration, the goal is to let feature create a timestamp outside of the Event Write Context. The configuration will be revisited when doing the split.Persisting
DatadogV1ContextUsageDatadogV1Contextstill exists for Crash-Reporting and WebView-Tracking integrations which do not use the Event Write Context.Review checklist
Custom CI job configuration (optional)