RUMM-2172 Single Storage #932
Conversation
| fun currentMetadata(): ByteArray? | ||
|
|
||
| /** | ||
| * @param event the event to write |
There was a problem hiding this comment.
minor: missing descriptions of the method and newMetadata arguments
| /** | ||
| * Holds information about the current local and server time. | ||
| * @property deviceTimeNs the current time as known by the System on the device (nanoseconds) | ||
| * @property serverTimeNs the current time synchronized with our NTP server(s) (nanoseconds) |
There was a problem hiding this comment.
| * @property serverTimeNs the current time synchronized with our NTP server(s) (nanoseconds) | |
| * @property serverTimeNs the current time synchronized with Datadog NTP server(s) (nanoseconds) |
| ), | ||
| ProcessInfo(forge.aBool(), forge.anInt()), | ||
| NetworkInfo(forge.aValueFrom(NetworkInfo.Connectivity::class.java), null), | ||
| UserInfo(null, null, null, emptyMap()), |
There was a problem hiding this comment.
should have nullable values forged
There was a problem hiding this comment.
Yes I know, I'll deal with those in a future PR (when those are actually needed)
| assertThat(RumFeature.initialized.get()).isEqualTo(rumEnabled) | ||
| assertThat(WebViewLogsFeature.initialized.get()).isEqualTo(logsEnabled) | ||
| assertThat(WebViewRumFeature.initialized.get()).isEqualTo(rumEnabled) | ||
| // assertThat(CoreFeature.initialized.get()).isTrue() |
There was a problem hiding this comment.
I would propose to add a TODO at the beginning of this file and DatadogCoreTest as well saying we should handle those commented lines (unlikely we will forget, but still)
| return | ||
| } | ||
|
|
||
| val file = orchestrator.getWritableFile(event.size) |
There was a problem hiding this comment.
thought: in this particular case callback is invoked synchronously, but in general API definition says it may be invoked in async manner, meaning that by the time it is invoked we may have other consent -> different orchestrator, not the one we captured for invocation. We need to be aware about that.
There was a problem hiding this comment.
(...) by the time it is invoked we may have other consent -> different orchestrator, not the one we captured for invocation
Not sure if I understand this @0xnm 🤔💭. We get the orchestrator based on the immutable datadogContext passed as an parameter to writeCurrentBatch(). Then, any time later we receive a callback which provides us API (BatchWriter) to writing data. The write strategy will respect the consent value from datadogContext.
From the caller's POV, IMO everything should be consistent with this implementation: the caller orders write for given consent value and it knows that BatchWriter will respect it. No?
There was a problem hiding this comment.
I mean that generally the API contract is that this callback may be invoked in async manner (if I got it right). My point is that what if user changed a consent in the timeframe between the moment we received an orchestrator and the moment we use it in the BatchWriter. Then event may be written to the location which belongs to the old consent.
This is quite hypothetical case (and may be quite an edge case), but this gist of it is that imo we need to keep in mind the possible SDK state changes with async invocations.
There was a problem hiding this comment.
This is an edge case that is not something we want to dig into. The time frame for such a concurrency issue will on most case something smaller than 500ms, which is totally acceptable.
There was a problem hiding this comment.
I get the point now @0xnm 👌. Agree, it's kind of edge case, but it can be clearly visible in such user situation:
// all synchronous, on a single thread:
Datadog.set(trackingConsent: .granted)
logger.info("granted log")
Datadog.set(trackingConsent: .pending)In such case, the "granted log" should be sent by the SDK. This is however a question on how we will further synchronise things in higher implementation (even higher than Storage level). The RFC recommends processing all user calls through context thread first (see Threading in DDCore) to support exactly these edge cases.
xgouchet
force-pushed
the
xgouchet/RUMM-2172/single_storage
branch
from
ddf1ba3
to
771eb8a
Compare
What does this PR do?
Provide an implementation of the single storage mechanism for SDK v2
Additional Notes
Unit tests are globally not passing but all tests on the new classes are green ✅