search

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

SwiftLog Compatibility (Issue) #111

Closed Sherlouk closed 2 years ago

Sherlouk commented 2 years ago

Hi there 👋

swift-log is a package by Apple which allows for extensive logging within applications and packages. I'd love to continue using this but route errors through to this Diagnostics package - the best of both worlds!

In order to do this, we can create a custom "LogHandler" and simply route the data through to the existing DiagnosticsLogger. Fabulous.

Two issues at the moment though:

  1. file/function are expected to be of type "StaticString" but Swift Log simply uses "String". It would be nice to change the signature here to allow for more flexibility in where the data comes from.
  2. the error type requires conforming to Error which is not always possible (well, isn't required by Swift Log). To get around this I've created a custom DiagnosticsError struct which accepts the contents of the SwiftLog message payload which does get around this but it would be awesome if the package allowed us to simply provide a string and potentially even metadata*.

Support for metadata in logging messages would be handy. I'd expect this to be something that is shown to the developer if they tap on that line in the HTML report generated. It would then show a simple table with key/value pairs allowing for more context on the error.

Hope this makes sense!

AvdLee commented 2 years ago

Interesting thought, it would not be bad to make Diagnostics more flexible in this regard!

file/function are expected to be of type "StaticString" but Swift Log simply uses "String". It would be nice to change the signature here to allow for more flexibility in where the data comes from.

Dang, we just moved away from a regular String! It shouldn't be hard to revert it back I guess. Though, have you tried creating a static string instead from what you get out of swift-log? If it's not possible, would it make sense to log this as file/function or should we create a different metadata destination for these?

the error type requires conforming to Error which is not always possible (well, isn't required by Swift Log). To get around this I've created a custom DiagnosticsError struct which accepts the contents of the SwiftLog message payload which does get around this but it would be awesome if the package allowed us to simply provide a string and potentially even metadata*.

Interesting, never thought about this scenario, but it makes sense to allow sending Strings as errors and make them appear as such. Feel free to contribute and make this possible!

Support for metadata in logging messages would be handy. I'd expect this to be something that is shown to the developer if they tap on that line in the HTML report generated. It would then show a simple table with key/value pairs allowing for more context on the error.

This is an interesting idea, but the doubt I have is discoverability. I'm not sure yet how this should work. Do you have an example or inspiration you got this from?

Sherlouk commented 2 years ago

Dang, we just moved away from a regular String! It shouldn't be hard to revert it back I guess. Though, have you tried creating a static string instead from what you get out of swift-log? If it's not possible, would it make sense to log this as file/function or should we create a different metadata destination for these?

It's not possible to get a StaticString from the swift-log library, for the time being I've simply prefixed the string into the message so it's like "MyFile.swift:42 - Some message has been set oh hello".

Interesting, never thought about this scenario, but it makes sense to allow sending Strings as errors and make them appear as such. Feel free to contribute and make this possible!

💯 Still playing around with the implementation and the app at large but will 100% come back round to this.

This is an interesting idea, but the doubt I have is discoverability. I'm not sure yet how this should work. Do you have an example or inspiration you got this from?

Not really, just kind of what I would maybe expect given the data available from Swift Log. I'm happy to raise a separate PR to show a bit of an example and we can take the conversation from there!

AvdLee commented 2 years ago

A fix is on its way: https://github.com/WeTransfer/Diagnostics/pull/118

github-actions[bot] commented 2 years 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.