steamclock / steamclog-swift Goto Github PK

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.

Now that #34 has been merged in, we need to be able to pull in the archived log files. XCGLogger's AutoRotatingFileDestination provides a method called archivedFileURLs(), we just need to expose it via SteamcLog

It would be nice to track log events by their build type to be able to filter on production builds, etc.

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/

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.

Currently it looks like any intentional crashes from clog.fatal / fatalError don't get the text in the fatal error function call attached to them (in any way I could find).

Seems like it should be possible for this to work, and is maybe just something we have misconfigured, and even if not, we should be able to work around it by just making sure a breadcrumb with the fatal error text gets added right before the crash.

Now that we've forked off the Android repo, we should add a proper .gitignore file.

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.

Right now this feature is doing a bit of work that can cause the UI to lock up. Move it into more of an async flow.

Right now when creating errors we use the trick of making the domain the text of the error and the code a fixed value. This is a bit of a kludge, and it'd be better to do those in a slightly cleaner way, though we need to decide on an exact design for this.

Some suggestions for the domain:

• The domain could just always be "steamclog.error" or something similar.
• The domain could be supplied with an optional parameter at the callsite, and default to steamclog.error
• The domain could automatically be based on the calling #filename and #function or similar.
• The domain could be based on the calling module (not 100% sure how we get that though)

Some suggestions for the code:

• A hash of the file and line number (doesn't stay stable across code changes though)
• A hash of the text (would cause merging of errors that happen to have unique text, unless th domain is unique in some way for uses of the same text in different files)
• A hash of some combination of filename, function name AND text (could still not be stable for some code changes, but mostly both stable and unique for all calls).

Currently, all crashes coming from clog.fatal calls are bundled together in Crashlytics because the location of the crash is the same. This also means that for a project using clog.fatal, only one Github issue will be filed for all of the clog.fatal calls together. As far as I can tell, there doesn't seem to be a simple way to fix this.

One possible work around would be to have clog.fatal call clog.error internally, then disable crash reporting and crash normally. This would allow us to fake the clog.fatal implementation that we're looking for, but it would also mean that the report generated would be a non-fatal report.

Comment from Jake on the first PR:

Something I hadn't thought of until now: Kotlin's default scope is public so it might make sense to do a pass at some point to convert everything to a more restrictive scope like internal?

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.

It seems like we just need to include it in the Config constructor.

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

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.

Now that we've forked off the Android repo, we should add a proper .gitignore file.

Right now crash / error reporting is either turned off in test builds (if we are using something more invasive like BugSee) or just report normally (which might not be supper useful if interesting new errors are just appearing in the general set from production). There are some things we can do to improve the current situation (#72, #83), but there are also other things we could do to make non-fatals in that environment MORE obvious.

For example showing an onscreen popup of some kind any time a non-fatal happens, with an immediate option to send detailed logs, maybe trigger a BugSee event if it's active, etc.

This would make it more likely to catch serious problems that don't cause visible bugs when they happen immediately, and up our chances of getting more useful error reports from internal or client QA.

• Check that the license files all point to steamclock and not individual people
• Write a proper readme
• Review documentation
• @nbrooke is going to split the repo in android/ios

In #17, we discussed adding a custom log level but found that we'd rather enforce strict presets. Because of this, we should remove the custom LogLevelPreset from the Android project

We use BugSee in some projects for test builds to more-invasive-but-more-useful error reporting, but Steamclog/Sentry doesn't always play well with it (can have problems if both Sentry and BugSee are on at the same time, if the project isn't using BugSee we probably DO want to be capturing Sentry info in test builds by default, etc.)

There's a broad range of potential solution here, ranging from just coming up with some guidelines here, all the way to potentially just integrating BugSee into Steamclog as well and making the default behaviour correct out-of-the-box.

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

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.

We are currently 2 major versions behind.

From the spec:

SCLog.track is not a log level, but a separate method to log data to an analytics destination. We decided that track was not a level in and of itself, and thus chose to exclude analytics logging from the levels cascading architecture. Alternatives to this approach include passing an analytics log message when logging at the info level. Although, it seems to make the most sense to keep track and info separate, as info logs (ie. application details) can be somewhat distinct from the data applied to analytics logging.

Once we have proper Firebase support up and running we can look into adding support for the track feature.

This came up as part of #4, it seems like we don't really need includeDefaultXCGDestinations to be configurable? It doesn't seem like it's necessary, but I'm not sure the reasoning behind including it initially.

From Nigel in #1:

One of the hard lessons from Spies launch is that error reports that include settable text are bad news (because if you use localizedDescription to generate that text you get reports for all territories, if you include user ids or similar you get multiple reports per user). I don't think we can totally eliminate that, but there are a couple tweaks to this we might want to consider to try and clamp down on that behaviour

• Have the "error" (and fatal?) entry points only take StaticString, so you CAN'T do dynamically built strings at that log level in the public interface.
• Have "error" entry points that actually take an "Error" object as well as just one that takes a string. That both gives us more information (we can use the real code and domain) and let's us construct the error in a away that is more consistent. Need to figure out how to get those through the system though, maybe in that case the crashlytics logging actually need to be at a higher level.
  In cases where we are making our own error, we should try and generate them in a way that is more consistent (we shouldn't use the message in the domain, probably we should set our own domain that's constant, and try and generate the code in some way based on the source location (we should have the file and line number, so if the code was a hash of the file and line number it should be the same for any given call, but different across calls, which I think is what we want). Message can always be included in the additionalUserInfo, where it shouldn't cause duplicate entries for the same error.

Android has this, and I think it'll make it much easier to migrate existing projects to SteamcLog in the future.

Especially for Crashalytics/GA.

By default Sentry doesn't seem to capture the callstack for recorded errors. For errors reported in commonly called functions this can make tracking them a little gnarly.

We should look into if there is a way to do this, or if not figure out if it's possible to get a little more context information ourselves (capture our own stack trace or something).

Having the documentation in Xcode is super helpful, we should go through and add these and make sure it's readable everywhere.

A bunch of the licence text (in LICENSE.md and the code) has copyright info that is pointing at Brendan's personal email address. Those should be changed to a Steamclock copyright, and definitely shouldn't have a personal email address exposed.

As discussed here, we're now allowing some more info to be passed along with fatal and error logs but it's not ideal.

We changed the definitions of the DestinationLevels after a follow up meeting, but Android is still using the original settings. Double check with iOS for the desired levels.

It would be useful to have a way to filter errors in the Sentry UI based on whether they happened in a debug or release build (and if possible distinguish TestFlight and AppStore errors in the release build at runtime).

It should be relatively straightforward to mark those with a tag in Sentry.

The automatic UI lifecycle breadcrumbs that Sentry produces seems to not always give great results when popups (and view controller containment?) are involved. For example, in Spies, almost any in-game crash or error is attributed to "CityDialogViewController", because it was often the last new view controller that was presented, even though it's generally been dismissed well before the actual problem happened.

We should see if there is something we can do to to make those results more reflective of what the actual state of the UI is currently.

Right now, when using the "user report" functionality, we still only get the log levels that are automatically sent to the breadcrumbs, which often is not enough for debugging. In some configurations, on disk logs are available with a higher verbosity and/or more history and would generally be included if the user was reporting something via email.

The privacy considerations are less in a situation where a report is intentional, because we can warn the user that we will collect some additional data and give them the chance to opt out.

The user repot method(s) should have the option to include the full logs from disk as a Sentry attachment.

When implementing this, it might also be worth generalizing the functionality so we can include other files as attachments rather than just doing this specific use case.

Currently on iOS, analytics are only tracked in config.logLevel == .release.

Currently the config file has a crashlyticsAppKey property, but we are not using it. We should determine how we want to handle setting up Crashlytics:

• Does the library need to worry about the appkey, or should this be on the onus of the application using our library?
• Test out end-to-end crash report

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?

Even if we don't want this enabled by default, we should at least add a mechanism to turn it on.

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.

Currently we have a lot of Android and iOS build files that aren't being ignored properly. We should add those to the gitignore and clean up the files.

Lots of (maybe all) times we are doing an analytics event we ALSO want to be logging something. We should tweak the analytics tracking so that it can automatically log the analytics events in some way as well.

Minimally, any analytics call should also (by default) cause the analytics event to get logged to the verbose logging level (though maybe there should be a way to turn this behaviour of globally and/or for a particular track call).

It might also be worth having some approach that we can easily log a warning/error and an analytics event in a single call. This could be either changing the analytics tracking method to take a log level (clog.track(id: "purchaseFailed", log: .warning)), change the warn/error methods to take a flag to enable analytics (clog.error("purchaseFailed", analytics:true)) or add some new methods specifically for this (clog.trackError("purchaseFailed")).

I think I mildly prefer the first option (analytics methods that takes a log level). It looks the cleanest to me, doesn't have the issue the other two do of maybe ending up with "bad" analytics events names if people are using normal log text as the event name, and has the nice advantage of also providing a reasonable way to turn off the default logging for a particular call with the same signature (clog.track(id: "eventWeDontWantToLog", log: .none))

It's useful to keep a basic analytics endpoint in SteamcLog so that we have one place to go to to make analytics calls, but unlike logging and crash reporting we can't really pick a fixed default (though we may pick a soft default, that can be overridden, eventually). We want some way to be able to plug in particular analytics tools depending on the project.

Basic design is to add an AnalyticsDestination protocol that basically provides the same endpoint as the main analytics call, have the ability to register AnalyticsDestination with the system, and basically just make the analytics call a pass through to the AnalyticsDestination object(s) that have been registered.

If it isn't merged by the time this is worked on, it probably makes sense to just do this on the sentry branch, since that branch has already disabled the old fixed default of Fabric Answers.

Calling fatal doesn't include the error in the title, which makes the stack-trace more confusing.