Firebase Crashlytics setCustomKeys stacked for all non-fatals

[peterplamenovpenchev]

[READ] Step 1: Are you in the right place?

Yes

[REQUIRED] Step 2: Describe your environment

• Android Studio version: Android Studio Bumblebee | 2021.1.1 Patch 2
• Firebase Component: Crashlytics
• Component version: 29.0.0

[REQUIRED] Step 3: Describe the problem

Steps to reproduce:

1. Have a function which records exceptions & sets custom keys

fun recordException(message: String, customKeys: Map<String, String> = emptyMap()) {
  val crashlytics = Firebase.crashlytics
  crashlytics.setCustomKeys { customKeys.forEach { key(it.key, it.value) } }
  crashlytics.recordException(Throwable(message))
}

2. Call this function several times in a single app usage session

recordException("Non fatal without keys")
recordException("Non fatal with keys", mapOf("key_1" to "value_1"))
recordException("Non fatal with another keys", mapOf("key_2" to "value_2"))

Result:
When the logs are flushed (at the next app start up) the custom keys are associated with all generated logs.
Effectively this means that in the steps above all non fatal logs have the key_1 & key_2 associated with them.

Another defect caused by the issue is that if the app generates a lot of logs with keys (via recordException) is that after the 64th customKey no other keys are added.

This produces erroneous log trail by the app & messes up the search by key feature in the Firebase Crashlytics console.

Expected result:
Based on this documentation I am left with the impression that the customKeys will be associated with the log they are fired for.

Note:

The iOS Crashlytics SDK has the capability of assigning keys for each log by using the userInfo in NSError, but that is not possible on Android (Java/Kotlin) because the Throwable does not provide such map (hence the SDK does not work in that way).

[argzdev]

Hi @peterplamenovpenchev, thanks for reporting. I was able to reproduce the same behavior, I'll notify an engineer and see what we can do here.

[mrober]

We are aware of this inconsistency between iOS and Android. We're considering making the two platforms consistent, but it's not a priority right now. We're hesitant because it would change a behaviour that many customers may currently rely on or expect.

Regarding the issue of dropping any keys after the 64th, switching it to retaining the most recently set keys seems like a good improvement and we've logged it as a feature request.

[peterplamenovpenchev]

Thanks! I am really looking forward into the changed behaviour of this aspect of the SDK.

@mrober is there a way to implement this on Android atm? I mean - having log specific keys?

[dperez37]

I also ran into this issue. As far as I can tell setCustomKey() does the exact same thing on both iOS and Android. So I do not think there is an inconsistency there.

The userInfo property that iOS uses to set specific properties for the logged exception is a missing feature on Android. I think the real solution is not to change the setCustomKey() functionality but to add an overload to the recordException method allowing us to pass in a map of other values for the specific exception.

Something like this:

public void recordException(@NonNull Throwable throwable, @NonNull Map<String, String> userInfo)

[peterplamenovpenchev]

Yea, this sounds very reasonable.
I went by the book & used setCusomKeys in order to achieve the log specific properties.
I think that what @dperez37 proposes makes a lot of sense.

Have you @mrober considered this or similar solutions? They won't change the existing behaviour.

[mrober]

Thanks for the suggestions. We will keep the user info as a feature request.

We discussed switching custom keys to retain the most recently set keys, but decided not to do it because it would change a subtle behaviour. For example, setting the most important custom key once at app launch.

[pistolcaffe]

I was confused by the same problem until I understood exactly how Crashlytics 's setCustomKey works. I think @dperez37 suggestion is a good idea. However, if the feature request is not possible, I think the Crashlytics documentation needs to be supplemented with a little more detail about setCustomKey and the Key notice displayed in the Firebase Console.

[almozavr]

Any update, roadmap? non-fatal API errors in crashlytics are mostly unusuable without custom keys record method, I'm shocked how iOS SDK is much better than Android 😳

[novag]

+1 for @dperez37's suggestion. I think this would be much cleaner and more expressive in many situations.

[jonasvogel]

I'm also quite surprised that the Android SDK does not allow adding metadata to a single non-fatal exception, just like @almozavr. Coming from iOS, extending non-fatal logs with additional infos via NSError's userinfo is one of the best things about Crashlytics. 😻

Any insights on the product decision not to add the overload as @dperez37 proposed? I mean, technically, everything is already there I suppose, otherwise it wouldn't work on iOS. 🤔

[gitabssi]

We are facing the same issue, as we have a team working on iOS app, and can attach custom metadata to a specific error, but for Android we can't.

[NicolasEymael]

we have the same error in our app

[mrober]

We discussed this with the team, and have decided to pull this work to add userInfo to logged exceptions like @dperez37's suggestion. I will post here again when we have an update.

[agaluzzi]

@mrober Can you please provide an update on this? It seems like an important issue to get resolved, since it makes the custom keys somewhat unusable.

The Crashlytics documentation specifically states that custom keys should be used for values that are unique to a specific exception. However, this still seems very broken, as it appears that the values are not captured at the time recordException is invoked.

While this suggested improvement of being able to pass key/value pairs as an argument might help, I don't believe it really solves the problem. Because setCustomKeys is intended for including "the specific state of your app" in crash/error reports, I think it ought to capture that state at the time the exception is recorded. Otherwise, the report may include incorrect state that was modified after recordException was called.

[nullptr2this]

@agaluzzi It's been a while since I looked at this, but I believe the custom keys recorded in setCustomKeys are captured loosely at the time of the exception. If memory serves, it's not synchronous as the work to copy the custom key set is run on a dispatcher or an executor (again it's been a while), and it's hard to tell when you can "unset" a custom key that is only related to a specific exception as there are no callbacks to indicate when an exception has officially been recorded. I'm not sure we going to get any movement on this any time soon, but if anyone from firebase responds to anything on this I'll go handle the merge conflicts on the PR. Otherwise, I have mostly given up on this.

[tejasd]

An API overload is now available to address this issue.

Marking this as closed.