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

Allow injecting different OSLog into the framework #18

Open
2 tasks
edorphy opened this issue Dec 21, 2021 · 5 comments
Open
2 tasks

Allow injecting different OSLog into the framework #18

edorphy opened this issue Dec 21, 2021 · 5 comments

Comments

@edorphy
Copy link

edorphy commented Dec 21, 2021

Hey Gui,

  • Developers should have the ability to disable the os_log in production builds to protect user data.
  • Developers should have the ability to customize the subsystem to their own need

Both can be achieved by providing a logging factory mechanism that can be overridden.

These can both be achieved by creating a factory or configuration passed into the transceiver at construction time. What are your thoughts on this as a privacy enhancement?

First thing I do when logging is make an extension on OSLog or Logger types that just take a category, the subsystem is usually the main bundle identifier.

@insidegui
Copy link
Owner

Hi @edorphy, I definitely agree that it would be beneficial for the framework to provide a way to customize the logging parameters such as the subsystem.

I'm not sure about this one though:

Developers should have the ability to disable the os_log in production builds to protect user data.

os_log has built-in mechanisms to redact private data. If you believe MultipeerKit is using %{public}@ in places where it shouldn't, then please let me know or submit a PR changing those over to %@, which will then get replaced by <private> if you view logs produced by a release build of the application.

Due to os_log having that safeguard, I don't think it's that important to allow for disabling logging entirely.

@edorphy
Copy link
Author

edorphy commented Dec 28, 2021

Hey @insidegui,

Generally speaking, I don't like logs of any sort being emitted that I'm not 100% in control of, this would at least put the developer using the library in control. But ultimately up to you; it is a forced habit of mine from my 9-to-5.

As for potentially leaking implementation details, I can review usage of %{public}@ and make a second issue/PR if I find anything that feels to be unnecessarily disclosing information.

@insidegui
Copy link
Owner

Sounds good, please do report if you notice any logging that's leaking private data in production builds.

The reason why I don't think it's necessary for my own work is because os_log has zero impact in the app when running on an end-user device, unless the Console app is actively streaming logs or there's a special profile installed on the device to enable logging persistence for a particular domain. The whole point of os_log is that you can leave it enabled in production builds in order to collect valuable debugging information when needed, but it won't affect the app during regular usage.

That said, I'm not against letting users of the library disable logging altogether either, just gotta think of a way to do it that doesn't require me to change the way I use os_log.

@edorphy
Copy link
Author

edorphy commented Mar 12, 2022

Hey @insidegui

Thoughts on changing the interface of the logger to Apple's other type from here: https://github.com/apple/swift-log? I know Apple says "Don't wrap around OSLog" because you'll lose items like line number and such, however the current day console app doesn't print that information (to my knowledge).

Your base implementation could use OSLog as the backing store, but it would allow flexibility if others (namely me) wanted to use something else. This is what I plan on doing eventually in the CloudKitWebServices library, as well as other items I'll eventually open source.

@insidegui
Copy link
Owner

I don't want to add an external dependency to MultipeerKit at this point. If you'd like to use SwiftLog, I'm afraid you'll have to create a separate fork.

I'd like to move over to the more modern Logger API though, and allow for a custom instance to be injected into the library.

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

No branches or pull requests

2 participants