WeTransfer / Diagnostics

Allow users to easily share Diagnostics with your support team to improve the flow of fixing bugs.
MIT License
933 stars 53 forks source link

MetricKit can cause apps to crash in some versions of iOS if there is more than one subscriber #142

Closed SwiftNativeDeveloper closed 10 months ago

SwiftNativeDeveloper commented 1 year ago

Hey team,

I really want to use your library, but I don't think I can as is because of the bug described here: iOS16 [MXMetricManager deliverMetricPayload:] Crash

In short, if there are more than one subscriber added to the MXMetricManager, it can cause the app to crash if it receives payloads. Looks like this was reported in 16.0 and fixed in 16.1 according to Eskimo @ Apple. To account for this I have a single place that receives payloads and then redirects them to the other 'recorders'. Your library would be one of them.

extension ApplicationReporter: MXMetricManagerSubscriber {
    func didReceive(_ payloads: [MXMetricPayload]) {
        // Explicitly do this on our worker queue to free MetricKit to do its thing
        self.queue.async {
            self.record(metricPayloads: payloads)
        }
    }

    func didReceive(_ payloads: [MXDiagnosticPayload]) {
        // Explicitly do this on our worker queue to free MetricKit to do its thing
        self.queue.async {
            self.record(diagnosticPayloads: payloads)
        }
    }
}
extension ApplicationReporter: DiagnosticReporter {
    func record(diagnosticPayloads: [MXDiagnosticPayload]) {
        for diagnosticReporter in diagnosticReporters {
            diagnosticReporter.record(diagnosticPayloads: diagnosticPayloads)
        }
    }
}

Could you expose a mechanism to allow payloads to be passed to the logger instead of you subscribing to it directly? I have accounted for this by replaying the payload callbacks to different 'reporters'.

Setup Function Subscriber added here

I would simply make a wrapper to replay the message to the library but I can't work around the fact that it is internally subscribing to the MXMetricManager inside the static setup function.

This is something I'd be willing to contribute back if we could agree on an approach. As it stands now, the simplest thing would be to pass in an optional boolean to the static setup which skips adding a subscriber and then expose the record method publicly to be invoked by the above sort of code. Since that is a proposal to the main startup function of the library I hesitate a little.

Got any other ideas or proposals to account for this issue confirmed by Apple?

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

SwiftNativeDeveloper commented 1 year ago

Comment to prevent from closing.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

SwiftNativeDeveloper commented 1 year ago

Bump

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

SwiftNativeDeveloper commented 1 year ago

I verbally confirmed with the MetricKit team at WWDC23 that the issue was fixed in 16.1. My understanding is that apps running [16.0-16.1) would be subject to this issue.

Is it worth decoupling the start monitoring call from the initializer to allow app developers to put special handing in for 16.0.x?

github-actions[bot] commented 11 months ago

This issue is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.