steamclock / steamclog-android Goto Github PK

Since we no longer want to use Firebase Crashlytics as our crash reporting system AND because issues were found while attempting to use only Sentry on a client project, we should just remove the previous Crashlytics support and force the few existing apps we have using steamclog to move to Sentry.

It's been awhile and some libraries are out of date.

When we attempt to create the FileWriter after retrieving the external file, we get a FileNotFoundException. It could be that I'm missing some configuration.

Before we make public we should look at creating a signing key for the library (do not store in repo) and sign our AARs with it. How does this work with hosting via Jitpack? Maybe we don't even need a signing key if hosted on jitpack?

Here is a sample file: https://www.figma.com/file/LKQ4FJ4bTnCSjedbRpk931/Sample-File

Automatically generated from steamclock/steamclog-swift#78

If we're logging too much, it can cause weird bugs and the system to lock up which is bad.

We should look into adding some mechanism that will detect if we're logging too much to the file system and fire off a warning/error.

Currently if we have extra data, there is a chance it may be appended to the title of the Sentry ticket.

This is cluttering up the page and makes tickets harder to read.
We should look into ways to make it so the extra data is always added to the logs as breadcrumbs.

Automatically generated from steamclock/steamclog-swift#71

From a discussion in Slack:

User reports build up the report based on the text the user enters, but switching them to use the static string doesn’t work great, because it means all user reports will fold into the same error, which is not ideal for noticing or quick filing bugs.
Question is: do we just switch them to use static string anyway for now, and live with it being a bit more complicated to work with them. Or do we add some dynamic endpoint back into SteamcLog just for that (but that slightly defeats the purpose of trying to force people to use static strings and do it right, if there’s just an alternate endpoint that lets them do it wrong)?
The solution might be to have a dynamic non-fatal endpoint, but specifically name it “userReport” or something. That feels better then just going back to having some more generic “dynamicError” call, might be a little less likely to be accidentally misused. That function can eventually do a little bit of extra work. For example take a user info dictionary (with things like their device id for Spies), that it turns into tags on the sentry report, automatically collect a little more device info, attach the full text logs like we get with email reports, etc.

Automatically generated from steamclock/steamclog-swift#34

Currently SteamcLog doesn't have the ability to rotate out log files, which is causing issues like https://github.com/steamclock/spies/issues/3959.

On iOS, we can use some of the built-in XCGLogger functionality, but Android may require more work.
XCGLogger provides AutoRotatingFileDestination with the following options:

targetMaxFileSize: Auto rotate once the file is larger than this (Default is 1_048_576)
targetMaxTimeInterval: Auto rotate after this many seconds (Default is 600)
targetMaxLogFiles: Number of archived log files to keep, older ones are automatically deleted (Default is 10)

At the very least, it seems like targetMaxTimeInterval would be useful for a first implementation.

@ssawchenko Do you know if Timber has similar functionality?

Implementation proposal:
Add an optional struct to the Config struct called FileAutoRotateParams (or something similar/better). Inside of that we can define rotateAfterSeconds and use that for creating the AutoRotatingFileDestination.

Found an issue in another project where we could not redact an optional property as the redacted list was expecting non optionals.

This may just have to be a limitation of using redaction in projects (ie. cannot be optional values)

Right now the default release and releaseAdvanced presets are not including enough data to be useful when debugging common crash reports.

We have discussed making the following changes:

• Change Release to log at info
• Change ReleaseAdvanced to log at verbose

Last version added a check on Debug Builds that did a run time check to ensure that the StackTrace we are creating for console logs was logging at the correct stack index. I was not applying the proper proguard rules for this though, and any Debug apps running would crash because the method name being used to apply the check was being munged.

New version available; update and do any required migrations

Right now our interface accepts "Any" object, and uses the config.requireRedacted property to decided if we are allowed to log the object or not.

Double check with iOS to see if iOS ONLY allows Redactable objects to be logged.

Automatically generated from steamclock/steamclog-swift#54

This came from a conversation in https://github.com/steamclock/spies/issues/4065. Right now, by switching to SteamcLog, most of out Crashlytics logging is lost due to the higher logging level.

Not sure when it happened, but the library package name reads com.example.lib, which is not desired. Change library package name to com.steamclock.steamclog

We haven't really discussed version codes (usually handled by Bitrise) or names (v1.2 vs 2021.1).
Let's talk with the team and finalize these! Then make sure that they will match what we are tagging in our release steps in the Readme (I think I actually missed doing this)

Create separate method that does not include FirebaseAnalytics instance

Firebase allowed us to add a user id to all subsequent reports. See if Sentry allows us to do the same.

Currently only error calls that include a throwable are being logged as non-fatals to crashlytics. We want ALL error calls to do this, so we will need to create a dummy/placeholder exception/throwable to provide this functionality.

Was removed when we moved over to using Sentry for Crash reporting. It would be nice to get this functionality back if possible.

See:

• steamclock/steamclog-swift#96
• https://coda.io/d/Tech-Working-Groups_dq8QHMnEi6A/Aug-16th-2021-SteamcLog-Review_suivT#_luYzz

Automatically generated from steamclock/steamclog-swift#77

We've run into problems on multiple projects now where the release default not logging anything to disk has made issues harder to diagnose and fix. It may be a good idea to reconsider this and at least log warn and above to disk, although I think an argument could be made to include info as well.

Allows us to add keys like server type etc...

iOS set to Firehose, Android currently set to Develop

From iOS readme

// defined globally
#if DEBUG
private let config = Config(logLevel: .firehose) // this will be used in debug builds.
#else
private let config = Config(logLevel: .release) // this will be used for release builds
#endif

As discussed in today's TechWG we are moving forward with migrating from Firebase Crashlytics to Sentry for error reporting:
https://docs.google.com/document/d/12Uj_m03NT0qmVfoiHXuO-rcadLX_FfMV26C_yKPvUKA/edit

Action Items:

• Add Sentry support in sclog library
• Test Sentry support using sample app provided in this repo
• Once this is all working look at adding to a client project

Automatically generated from steamclock/steamclog-swift#75

Automatically generated from steamclock/steamclog-swift#49

Dynamically generating these causes all kinds of problems with error deduplication, so we should just force them to be static.

I'm not totally clear if this is also doable in Kotlin but if it is, we should do it on Android as well.

We should update our README with usage instructions etc. (similar to the steamclog-swift README)

Automatically generated from steamclock/steamclog-swift#82

I'm not sure why we decided against this originally, but since we're able to determine which build a report comes from in Sentry this seems like it makes sense to me?

Specifically, make sure to test that when we are using NonFatalException, the offset used for numToRemove puts us in an expected location.

Note, since NonFatalException existed only to support Crashlytics, once we remove support for it fully we will not need to test it.

1. Import library in the gradle file.
2. Replace all Timber.X calls with related SCLog calls
3. Non-fatals are currently logged by the CrashlyticsReporter.captureNonFatal method. May be worth having a sit down/pair program to figure out the best way to replace the existing Reporter interface with SCLog.
4. Analytics currently logged by the AmplitudeAnalytics.log method; again, it may be worth a having a pair progam sit down to discuss how to replace this properly.

This happened on VB - we were reporting non-fatals when calls failed due to a network connection issue. In most cases, this is not a case where we'd want to mark a non-fatal (unless it produced a situation that would impact the user).

May want to check the Throwable before being logged, and only log if not an IOException

Changes made to LogLevelPresets, update Readme to match

Automatically generated from steamclock/steamclog-swift#85

We are currently 2 major versions behind.

Automatically generated from steamclock/steamclog-swift#50

In cases where we have an underlying error available,we should be able to pass it straight through rather than needing to generate a string ourselves from it (which has issues with localization, and is just more work).

We should definitely add this for the error level, and maybe consider it for the warn and fatal levels as well. Those wouldn't be as useful under Crashlytics, because we wouldn't want to report Crashlytics non-fatals for warnings or fatals, but assuming we do eventually move to Sentry, it has enough better categorization that it might be useful to send errors with a different tag on other levels for those that can be organized separately (for example, network errors could be sent at the warn level, giving the ability to track the frequency, how we do sometimes now by making network errors an analytic event, of those while still not having them generate issues automatically like errors do.

One design element that needs to be resolved before doing this: do we want to accept ALL errors, or do we want to force errors to opt in in some way, so that we can try and enforce the same redaction rules as we do for passing in objects.

On android the class involved isn't Error (java.lang.Throwable maybe?), but we shoudl do the same thing.

Deprecated; replace with view binding

Using the following config, it seems like Sentry is still sending bug reports:

clog.initWith(isDebug = BuildConfig.DEBUG, fileWritePath = externalCacheDir)
if (!BuildConfig.DEBUG) {
    clog.config.logLevel = LogLevelPreset.Release
}

From #53

There's a quirk with how Sentry groups reports that contain Throwables:

Grouping by Stack Trace
When Sentry detects a stack trace in the event data (either directly or as part of an exception), the grouping is effectively based entirely on the stack trace.

This "grouping" seems to assume that the message names for the grouped Throwable is the same - this led to a case in my sample project where I was using a generic Throwable with multiple messages in the same function that were becoming grouped on the Sentry dashboard, with state of the message seeming to change based on the last report made.

iOS Parity
Check and see if these "User reports" on iOS also contain stack trace info.

• If yes, then we may need to look at adding stack trace data IN THE LOGS only
• If no, then I believe the Android library already supports this by allowing an error to be created using just a message string

Automatically generated from steamclock/steamclog-swift#79

Sometimes we'd like to attach meta info as tags to reports that doesn't make as much sense to provide in the actual log description.

https://docs.sentry.io/platforms/apple/guides/ios/enriching-events/tags/

Automatically generated from steamclock/steamclog-swift#76

This would either stop further potentially spurious nonfatals from being sent, or at least flag them as being "post-integrity failure" and maybe spurious

Automatically generated from steamclock/steamclog-swift#7

We will need to:

1. Add a licence file to the app
2. Determine how to get the library up and available on either jitpack (or maybe maven?) for us easily use in our projects

While integrating with a client project I forgot to extend my data classes with Redactable, this resulted in the Sentry report simply not being set. As it took me awhile to figure out this was the issue, we should look into (1) allowing non-Redactable objects to be attached (not sure if iOS allows this) or (2) log a separate error (console, maybe even non-fatal) to track that this case has occurred.

Go over each Destination and make sure that log levels are being adhered to.