bitdriftlabs / capture-sdk

mobile bitdrift SDK
Other
14 stars 4 forks source link

Support real lazy evaluation of the log message lambda #60

Open murki opened 1 month ago

murki commented 1 month ago

Two potential improvements to the logging interface which currently accepts a message: () -> String lambda:

  1. Reduce the number of memory allocations. The way it is today the code will allocate once for the lambda, and then again for the string. The ideal way to solve this would be to use inline in the method but since we use interfaces in between that's currently not possible. We should at least provide an overload that just takes the String directly for cases where the consumers know there won't be any string interpolation.
  2. Accepting the lambda can give the idea to consumers that the code would be conditionally executed based on the matcher rules. Since we put every log in the ring buffer we should at least add documentation around the expectations on the execution of this lambda. The only times the lambda wouldn't be executed is if the SDK is not initialized.
pyricau commented 1 month ago

This is very much a blocking point for us.

We would need the ability to send all of our logcat {} (same as Timber) logs to bitdrift, with the assurance that the strings aren't even evaluated by default / the lambdas don't run, because that can have a significant performance impact. So based on some tag and server side rule we'd want to conditionally evaluate.

The lambda approach is actually inlined syntactic sugar on 2 basic API building blocks: 1) "should I evaluate this log?" and 2) "here's my log"

pyricau commented 1 month ago

Once that top level API is available, we would benefit even from a new API that doesn't take a string but an Okio Source instead (similar to Java InputStream), so that we could serialize some event classes directly to a json string stored in the ring buffer without creating temporary strings per log.

Although thinking about this more, this might not work because you require knowing the log size when allocating an entry in the ring buffer. so, that might be a bad idea, or might need an API that includes the size.

Augustyniak commented 1 month ago

Some notes with regard to lambdas evaluation:

  1. The lambdas are not evaluated for cases when the SDK is not configured (no Logger.configure call).
  2. Conditionally evaluating lambdas based on specific matching criteria would mean evaluating them on the SDK's background thread. This would require all lambdas to be thread-safe. For example, your lambdas could no longer access UI properties (at least on iOS), as they would be evaluated on a background thread, and accessing UI properties from non-main threads is not allowed.

Maybe we could come up with a solution where you can log either by using a lambda or by passing a string/fields directly to the SDK. This would allow deferred evaluation of logs when needed, while keeping things simpler for cases where you're fine with always evaluating the log and its fields.