-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add app launch TTI log #12
Changes from 12 commits
81d68e5
572d7f1
4f2d648
150da31
dd0ca45
b3d50a2
6a5b61a
66a8134
f47b8ed
d80b904
ca3cd42
163f51c
0b33aa4
0454768
8981467
a4d8463
50508c8
c31c759
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,6 +229,19 @@ internal object CaptureJniLibrary { | |
durationS: Double, | ||
) | ||
|
||
/** | ||
* Writes an app launch TTI log. The method should be called only once per logger Id. Consecutive calls | ||
* have no effect. | ||
* | ||
* @param loggerId the ID of the logger to write to. | ||
* @param durationS the time between a user's intent to launch the app and when the app becomes | ||
* interactive. | ||
*/ | ||
external fun writeAppLaunchTTILog( | ||
loggerId: Long, | ||
durationS: Double, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also worth mentioning that the value should be positive otherwise will be ignored There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I like the idea. Will do.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
) | ||
|
||
/** | ||
* Flushes logger's state to disk. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ package io.bitdrift.capture | |
import io.bitdrift.capture.events.span.Span | ||
import io.bitdrift.capture.network.HttpRequestInfo | ||
import io.bitdrift.capture.network.HttpResponseInfo | ||
import kotlin.time.Duration | ||
|
||
/** | ||
* A Capture SDK logger interface. | ||
|
@@ -72,6 +73,7 @@ interface ILogger { | |
|
||
/** | ||
* Logs a message at a specified level. | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fighting the good fight There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. always for the right cause |
||
* @param level the severity of the log. | ||
* @param fields and optional collection of key-value pairs to be added to the log line. | ||
* @param throwable an optional throwable to include in the log line. | ||
|
@@ -84,6 +86,15 @@ interface ILogger { | |
message: () -> String, | ||
) | ||
|
||
/** | ||
* Writes an app launch TTI log event. This event should be logged only once per Logger configuration. | ||
* Consecutive calls have no effect. | ||
* | ||
* @param duration The time between a user's intent to launch the app and when the app becomes | ||
* interactive. | ||
*/ | ||
fun logAppLaunchTTI(duration: Duration) | ||
|
||
/** | ||
* Signals that an operation has started at this point in time. Each operation consists of start | ||
* and end event logs. The start event is emitted immediately upon calling the | ||
|
@@ -100,12 +111,14 @@ interface ILogger { | |
|
||
/** | ||
* Records information about an HTTP network request | ||
* | ||
* @param httpRequestInfo information used to enrich the log line | ||
*/ | ||
fun log(httpRequestInfo: HttpRequestInfo) | ||
|
||
/** | ||
* Records information about an HTTP network response | ||
* | ||
* @param httpResponseInfo information used to enrich the log line | ||
*/ | ||
fun log(httpResponseInfo: HttpResponseInfo) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,9 @@ pub mod error; | |
pub mod metadata; | ||
|
||
use bd_client_common::error::handle_unexpected; | ||
use bd_logger::{log_level, AnnotatedLogField, LogField, LogFieldKind, LogType}; | ||
use bd_runtime::runtime::Snapshot; | ||
use parking_lot::Once; | ||
use std::future::Future; | ||
use std::ops::Deref; | ||
use std::pin::Pin; | ||
|
@@ -92,6 +94,7 @@ pub struct LoggerHolder { | |
logger: bd_logger::Logger, | ||
handle: bd_logger::LoggerHandle, | ||
future: parking_lot::Mutex<Option<LoggerFuture>>, | ||
app_launch_tti_log: Once, | ||
} | ||
|
||
impl Deref for LoggerHolder { | ||
|
@@ -109,6 +112,7 @@ impl LoggerHolder { | |
logger, | ||
handle, | ||
future: parking_lot::Mutex::new(Some(future)), | ||
app_launch_tti_log: Once::new(), | ||
} | ||
} | ||
|
||
|
@@ -158,6 +162,39 @@ impl LoggerHolder { | |
holder.shutdown(false); | ||
drop(holder); | ||
} | ||
|
||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just learning about this primitive and their docs say
which seems to be aimed to help with
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sg |
||
let duration_ms = duration.as_seconds_f64() * 1_000f64; | ||
if duration_ms < 0.0 { | ||
log::warn!( | ||
"dropping app launch TTI log: reported TTI is negative: {}", | ||
duration_ms | ||
); | ||
return; | ||
} | ||
|
||
let fields = vec![AnnotatedLogField { | ||
field: LogField { | ||
key: "_duration_ms".into(), | ||
value: duration_ms.to_string().into(), | ||
}, | ||
kind: LogFieldKind::Ootb, | ||
}]; | ||
|
||
self.log( | ||
log_level::INFO, | ||
LogType::Lifecycle, | ||
"AppLaunchTTI".into(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
fields, | ||
vec![], | ||
None, | ||
false, | ||
); | ||
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. kudos for not putting this logic in |
||
} | ||
|
||
impl<'a> From<LoggerId<'a>> for i64 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not accurate /trolling