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

ref(logging): provide alternative log output sinks #4088

Closed
wants to merge 6 commits into from

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Jun 18, 2024

I need alternative destinations for certain logging tasks besides just NSLog, like printf or files. This refactors the current logging API to allow for providing a combination of those three, defaulting to file and NSLog.

It converts SentryLog's API from class-bound methods to instance-bound, which simplifies a couple things in the tests and the codebase, and will allow for having more than one logger instance, so that the other places that need an alternative can set up their own logger.

The default logging setup is still provided via a singleton, which we already did implicitly, so this is just made more explicit.

Helps with #4020; #skip-changelog

TODO:

  • expose C functions for writing to printf() and files (using write()) so that contexts that require async-safety can call them directly, bypassing the objc apparatus. Need to figure out how to make this play nicely with configured log levels.

…om class-bound to instance-bound, allowing alternative loggers in certain contexts
Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentrySubClassFinder.m

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1207.21 ms 1217.72 ms 10.52 ms
Size 21.58 KiB 680.75 KiB 659.17 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b6ba04e 1230.48 ms 1253.20 ms 22.72 ms
a176fc4 1250.29 ms 1257.88 ms 7.59 ms
b4f8dba 1343.92 ms 1362.96 ms 19.04 ms
9ae806a 1243.84 ms 1256.22 ms 12.39 ms
afd1a08 1207.78 ms 1223.44 ms 15.66 ms
4bca912 1252.42 ms 1260.06 ms 7.64 ms
2ce582e 1238.98 ms 1259.66 ms 20.68 ms
45d3ca5 1238.53 ms 1263.09 ms 24.55 ms
aa4eddf 1228.21 ms 1236.72 ms 8.51 ms
5d6ce0e 1206.72 ms 1228.67 ms 21.95 ms

App size

Revision Plain With Sentry Diff
b6ba04e 20.76 KiB 414.44 KiB 393.68 KiB
a176fc4 22.84 KiB 403.24 KiB 380.39 KiB
b4f8dba 21.58 KiB 614.87 KiB 593.29 KiB
9ae806a 21.58 KiB 616.14 KiB 594.56 KiB
afd1a08 22.84 KiB 402.57 KiB 379.72 KiB
4bca912 22.85 KiB 411.14 KiB 388.29 KiB
2ce582e 22.85 KiB 410.97 KiB 388.12 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB
aa4eddf 21.58 KiB 544.86 KiB 523.28 KiB
5d6ce0e 22.85 KiB 405.38 KiB 382.53 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Is it possible to write the new classes in Swift?

@armcknight
Copy link
Member Author

Is it possible to write the new classes in Swift?

The next step was going to be factoring out the implementation into C functions that can be called outside of the objc path so we can use them from async contexts, so I'm not sure.

@armcknight
Copy link
Member Author

armcknight commented Jun 21, 2024

I'm going to close this and work on something slightly different related to logging, because this is getting too deep into file-based logging issues like log size/rotation etc that I didn't initially plan to take on. If we want file-based logging in the future, we should investigate forking an existing solution.

What I really wanted was to be able to use an async-safe logger from more of the profiling code, since there's still some stuff there that winds up using NSLog, which is unsafe from the logging thread.

What I'm going to do there is repurpose the crash reporter logger and reuse it there.

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.

3 participants