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

Add a metrics kit logger #89

Merged
merged 3 commits into from
Sep 3, 2021
Merged

Add a metrics kit logger #89

merged 3 commits into from
Sep 3, 2021

Conversation

AvdLee
Copy link
Contributor

@AvdLee AvdLee commented Sep 1, 2021

For now, we're only logging crashes as the other diagnostics aren't easy to read and won't add as much value as the crashes payload does.

Fixes #86
Fixes #73

@AvdLee AvdLee self-assigned this Sep 1, 2021
@AvdLee AvdLee requested review from amirdew and kevinrenskers and removed request for Boris-Em September 1, 2021 09:13
@wetransferplatform
Copy link
Collaborator

wetransferplatform commented Sep 1, 2021

Warnings
⚠️ Code coverage generation failed with error: Files encountered an error at 'build/reports/'. Reason: missing.
⚠️ Xcode Summary failed with error: Files encountered an error at 'build/reports/'. Reason: missing.
⚠️

Sources/DiagnosticsLogger.swift#L122 - It is safer to use weak instead of unowned

⚠️

Sources/DiagnosticsLogger.swift#L165 - It is safer to use weak instead of unowned

Messages
📖

View more details on Bitrise

Generated by 🚫 Danger Swift against 95c9596

}

NSSetUncaughtExceptionHandler { exception in
MetricsMonitor.logExceptionUsingCallStackSymbols(exception, description: "Uncaught Exception")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly we now log crashes the old way and new way through metric kit? Because metric kit is only delivering reports once every 24 hours.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! MetricKit only delivers once per 24 hours, so you're not sure whether a crash is reported. Though, MetricKit reports much more than this exception handler. For example, EXC_BAD_ACCESS crashes don't get catched with this handler as there's no exception thrown.

Therefore, a combination of both is most useful

extension MetricsMonitor: MXMetricManagerSubscriber {
@available(iOS 13.0, *)
func didReceive(_ payloads: [MXMetricPayload]) {
// We don't do anything with metrics yet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we open an issue for this? There might be some valuable information we could log as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked whether there was something obvious valuable, but there wasn't. Crashes really is the one thing we need now I think. Battery draining information and stuff won't really help in support issues.

Let me know if you disagree!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree. The only one that comes to mind is background termination reason and memory termination.

Copy link
Contributor

@kairadiagne kairadiagne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I have just two questions.

@AvdLee AvdLee requested a review from kairadiagne September 2, 2021 12:28
@AvdLee AvdLee merged commit d968efe into master Sep 3, 2021
@AvdLee AvdLee deleted the feature/log-metrickit branch September 3, 2021 13:47
@wetransferplatform
Copy link
Collaborator

Congratulations! 🎉 This was released as part of Release 1.10.0 🚀

Generated by GitBuddy

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

Successfully merging this pull request may close these issues.

Use MetricKit to better collect crashes Explore iOS 14 MetricKit possibilities
4 participants