Skip to content
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

Support real lazy evaluation of the log message lambda #60

Open
murki opened this issue Oct 3, 2024 · 4 comments
Open

Support real lazy evaluation of the log message lambda #60

murki opened this issue Oct 3, 2024 · 4 comments
Labels

Comments

@murki
Copy link
Contributor

murki commented Oct 3, 2024

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.
@murki murki added the android label Oct 4, 2024
@murki murki changed the title [ANDR] Support real lazy evaluation of the log message lambda Support real lazy evaluation of the log message lambda Oct 4, 2024
@pyricau
Copy link

pyricau commented Oct 4, 2024

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
Copy link

pyricau commented Oct 4, 2024

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
Copy link
Contributor

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.

@pyricau
Copy link

pyricau commented Dec 16, 2024

Conditionally evaluating lambdas based on specific matching criteria would mean evaluating them on the SDK's background thread

Why is that? Is that because the rule "should this be evaluated" is slow enough that it can't run on main?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants