Feat: allow logcat attachment to be previewed in Sentry

getsentry / sentry-dotnet

Sentry SDK for .NET

https://docs.sentry.io/platforms/dotnet

MIT License

599 stars 206 forks source link

Feat: allow logcat attachment to be previewed in Sentry #3711

Closed bricefriha closed 3 weeks ago

bricefriha commented 4 weeks ago

Context

As issue #3703 reports, Logcats files currently attached to Sentry issues are not previewable.

This issue is due to the mime-type sent with the file, as it currently has its own type, text/logcat. However, Sentry's preview component does not support this type. This PR aims to tackle just that by changing the mime-type we send from text/logcat to text/plain.

Caviat

I personally believe that the user should have a more console-like experience when looking at the preview on Sentry. To tackle this, an issue in Sentry's repo is currently open. So, the PR would offer more of a temporary fix than a complete solution. Therefore, when/if text/logcat gets support, we could change the mime-type back on our end

github-actions[bot] commented 4 weeks ago

	Messages
:book:	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by :no_entry_sign: dangerJS against 978e2314dbbb7ef6a3df89ddf3fbbf60da9246be

bricefriha commented 3 weeks ago

I can't find a way to get mime-type during device tests.

But it's not like we need to test this since the mime-type been hardcoded anyways

jamescrosswell commented 3 weeks ago

I can't find a way to get mime-type during device tests.

But it's not like we need to test this since the mime-type been hardcoded anyways

@bricefriha the device tests are just unit tests that get run on Android or iOS by our device runner here: https://github.com/getsentry/sentry-dotnet/blob/a2bdc824e133fee51fafcca6d792b466c456dca6/test/Sentry.Maui.Device.TestApp/Startup.cs#L22-L29

So you don't need to do anything special... just write some unit tests for LogCatAttachmentEventProcessor.Process.

Note: I see that method isn't very testable... it references static methods like OperatingSystem.IsAndroidVersionAtLeast so it might need to be refactored a bit.

bricefriha commented 3 weeks ago

have you actually tested this in an android app to make sure the logs come through OK and can be previewed in Sentry?

@jamescrosswell I have tested with the sample we have and it can be previewed in Sentry (as you can see on the screenshot in the description)