Diagnostics: Add diagnostics event types#783
Conversation
Codecov Report
@@ Coverage Diff @@
## diagnostics #783 +/- ##
===============================================
+ Coverage 81.54% 81.75% +0.20%
===============================================
Files 121 123 +2
Lines 3999 4045 +46
Branches 512 512
===============================================
+ Hits 3261 3307 +46
Misses 535 535
Partials 203 203
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| private const val VERSION = 1 | ||
| } | ||
|
|
||
| data class Log( |
There was a problem hiding this comment.
This type of event is not yet supported in the backend but I believe we will likely need it. We can remove it later if not.
| val name: DiagnosticsLogEventName, | ||
| val properties: Map<String, Any>, | ||
| val dateProvider: DateProvider = DefaultDateProvider(), | ||
| val time: Date = dateProvider.now |
There was a problem hiding this comment.
I used this mechanism to automatically add the timestamp which we use in SubscriberAttributes.
There was a problem hiding this comment.
I would rename this to dateTime, so time is accessed dateTime.time
| "\"name\":\"endpoint_hit\"," + | ||
| "\"properties\":{\"test-key-1\":\"test-value-1\",\"test-key-2\":123,\"test-key-3\":true}," + | ||
| "\"timestamp\":1675954145" + | ||
| "}" |
There was a problem hiding this comment.
@thecadams lmk if you think a body like this would be ok for sending logs to the backend. I can change it if needed though.
There was a problem hiding this comment.
Looks good.
Quick note - the real endpoint will not accept those test-key-{1,2,3} keys; have a look at https://github.com/RevenueCat/khepri/blob/main/khepri/controllers/diagnostics.py#L10 to see what it will accept.
(it was done in line with https://revenuecat.slack.com/archives/C04LXLZL8AX/p1676563654177199)
There was a problem hiding this comment.
Yup, this is only for unit tests. I separated the events properties a bit better in #803 so they are the ones we expect
| "\"exc_class\":\"TestClass.kt\"," + | ||
| "\"message\":\"test message\"," + | ||
| "\"location\":\"DiagnosticsEvent:121\"," + | ||
| "\"timestamp\":1675954145" + |
There was a problem hiding this comment.
Currently the backend expects a string, what do you think about sending the timestamp since epoch instead? This is in seconds, which I think is good enough. Lmk if you have any reason to prefer a string though @thecadams
| "\"version\":1," + | ||
| "\"type\":\"metric\"," + | ||
| "\"name\":\"test_metric_name\"," + | ||
| "\"tags\":[\"test-1\",\"test-2\"]," + |
There was a problem hiding this comment.
Just to let you know since the merge of https://github.com/RevenueCat/khepri/pull/5258 this now needs to be a hash, eg.:
"tags": {"tag1": "test-1", "tag2": "test-2"},
There was a problem hiding this comment.
Yup! Thanks for the reminder! I will update that in #805
Description
This is the first PR in a series of PRs to support sending diagnostic data from our SDKs to our backend. In this PR, we are just adding the models that we can potentially send to the backend.
This work is based on #766 but I'm splitting it up to make it easier to review. It's pointing to the
diagnosticsbranch so we don't merge to main until everything is ready.