add app launch TTI log #12
Conversation
| @@ -121,6 +124,8 @@ class MainActivity : ComponentActivity() { | |||
|
|
|||
| Logger.addField("field_container_field_key", "field_container_field_value") | |||
| Logger.logInfo(mapOf("key" to "value")) { "MainActivity onCreate called" } | |||
|
|
|||
| Logger.logAppLaunchTTI(1.toDuration(DurationUnit.SECONDS)) | |||
There was a problem hiding this comment.
this is not accurate /trolling
| */ | ||
| external fun writeAppLaunchTTILog( | ||
| loggerId: Long, | ||
| durationS: Double, |
There was a problem hiding this comment.
why the S in the param name? Is it in Seconds?
There was a problem hiding this comment.
I see below that it is. Is this a standard, to use seconds in Doubles to track time? and if so we should add the seconds unit part to the docstring
There was a problem hiding this comment.
also worth mentioning that the value should be positive otherwise will be ignored
There was a problem hiding this comment.
I see below that it is. Is this a standard, to use seconds in Doubles to track time? and if so we should add the seconds unit part to the docstring
I like the idea. Will do.
also worth mentioning that the value should be positive otherwise will be ignored
Is there ever a case when negative duration make sense? 🙃 I can add this info to the method documentation but I wonder whether it's worth it given that it will provide little to no value at the expense of more clutter in docs. Thoughts?
There was a problem hiding this comment.
Anything that an API allows users will do. My opinion is that we should never allow for invalid operations using the type system itself e.g. our Span API doesn't allow negative durations.
So at least we should document that this particular usage is invalid
| @@ -72,6 +73,7 @@ interface ILogger { | |||
|
|
|||
| /** | |||
| * Logs a message at a specified level. | |||
| * | |||
There was a problem hiding this comment.
always for the right cause
| false, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
kudos for not putting this logic in shared-core 🙌
so much easier to review and make changes if needed
| /// Logs an out-of-the-box app launch TTI log event. The method should be called only once. | ||
| /// Consecutive calls have not effect. | ||
| pub fn log_app_launch_tti(&self, duration: time::Duration) { | ||
| self.app_launch_tti_log.call_once(|| { |
There was a problem hiding this comment.
I'm just learning about this primitive and their docs say
This method will block the calling thread if another initialization routine is currently running.
which seems to be aimed to help with
It is also guaranteed that any memory writes performed by the executed closure can be reliably observed by other threads at this point
This sounds like an overkill to me since there are no shared resources that we'd like to guard, but maybe I'm misunderstanding something
There was a problem hiding this comment.
This method will block the calling thread if another initialization routine is currently running.
yes, makes sense. Basically there is a lock in here to guarantee that the critical path is run only once. I do not think that it's a problem, we use locks here and there.
This sounds like an overkill to me since there are no shared resources that we'd like to guard, but maybe I'm misunderstanding something
I think it's fine, we use it in similar cases in other places in code. We want to protect ourselves from cases where a user wants to log app launch TTI multiple times, with this method we can implement it with a minimal amount of effort + it protects us from cases where a user have multiple threads that try to log app launch TTI at the same time (yes, it will most likely not happen in real life scenarios but since we get this protection almost fore free it's nice to have)
| self.log( | ||
| log_level::INFO, | ||
| LogType::Lifecycle, | ||
| "AppLaunchTTI".into(), |
There was a problem hiding this comment.
It could be argued that this value should be just a field in the existing app launch event, but I see how exposing it in our public api would make that complicated, so I guess this will have to do for now
There was a problem hiding this comment.
yea, making it a part of existing TTI would be tricky. Also, I think that they may diverge in the past: app launch is app launch as defined by the system while app launch TTI may be time to interaction to the first time a user touches a screen (depending on what we mean by TTI) and having them combined then would be tricky. I agree tho, we can cross this bridge when we get there
Add predefined App Launch TTI log that can be emitted by customers using Capture SDK. In the future, we will create Capture SDK integration that emits this log automatically, in the current version we expect Capture SDK customers to call the method manually.