Skip to content

iOS public logging#408

Merged
KevinSchildhorn merged 3 commits intotouchlab:mainfrom
rocketraman:rg/log-public
Nov 15, 2024
Merged

iOS public logging#408
KevinSchildhorn merged 3 commits intotouchlab:mainfrom
rocketraman:rg/log-public

Conversation

@rocketraman
Copy link
Copy Markdown
Contributor

Change the implementation of native method darwin_log_with_type to use %{public}s rather than %s.

Resolves #400

Copy link
Copy Markdown
Contributor

@KevinSchildhorn KevinSchildhorn left a comment

Choose a reason for hiding this comment

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

Could you change it to add a function for public logging, or add a parameter?
We'd like the public logging to be an opt-in so that by default it wont be public.

@rocketraman
Copy link
Copy Markdown
Contributor Author

rocketraman commented Nov 14, 2024

@KevinSchildhorn I'm not sure that approach really makes sense in the context of Kermit. The reason is because when os_log is used directly, the default is for static strings to be displayed as public, and for dynamic strings to be private. In the context of Kermit, because we're delegating to os_log, everything is considered dynamic.

Users wouldn't expect the behavior of everything-private-by-default. They would expect logs to be public, just as they are on other platforms, and would elide dynamic data that should not be logged before sending it to Kermit. They would need to do this for the other platforms they are running on in any case. Therefore, I think it makes sense to use %{public}s by default, as that would offer consistency across platforms, and follow the principle of least surprise for users.

All that being said, I've done the implementation of opt-in public logging anyway. However, I can remove that second commit, or alternatively reverse the default, if you agree with my argument above.

Note also that this change will now conflict with #409. After that PR is merged, I can rebase this one.

@KevinSchildhorn
Copy link
Copy Markdown
Contributor

Thank you for adding the opt-in for public logging. We understand your argument and the expected behavior, however we don't want to change the default implementation silently at this time. There may be existing users of Kermit that have sensitive data in their logs, and we don't want to change their logs that they expect are private to suddenly be public.

@rocketraman
Copy link
Copy Markdown
Contributor Author

Thank you for adding the opt-in for public logging. We understand your argument and the expected behavior, however we don't want to change the default implementation silently at this time. There may be existing users of Kermit that have sensitive data in their logs, and we don't want to change their logs that they expect are private to suddenly be public.

Sounds good. I see you're already merged the conflicting changes into this branch. Were there any other changes needed here?

Copy link
Copy Markdown
Contributor

@KevinSchildhorn KevinSchildhorn left a comment

Choose a reason for hiding this comment

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

Should be good to go now, thank you for the PR and your understanding.

@KevinSchildhorn KevinSchildhorn merged commit 5f5fc07 into touchlab:main Nov 15, 2024
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.

OSLogWriter logs doesn't show up in console after process is detached from XCode

2 participants