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

It's time to upgrade RCTDefaultLogFunction to use modern os_log API #27863

Closed
LeoNatan opened this issue Jan 25, 2020 · 7 comments
Closed

It's time to upgrade RCTDefaultLogFunction to use modern os_log API #27863

LeoNatan opened this issue Jan 25, 2020 · 7 comments
Assignees
Labels
Bug Resolution: Locked This issue was locked by the bot.

Comments

@LeoNatan
Copy link
Contributor

Currently, the default logger outputs directly to stderr. This renders log collection using Apple's log stream API broken, as stderr output is not collected by the Apple logging system. For example, in Detox, we collect the device logs when running a test, but anything logged from the JS is not collected.

It's time to upgrade this behavior by using the Apple Unified Logging API. As an added bonus, the system logging API has log levels similar to those of RCTLogLevel, which can be mapped to the system log levels.

I am willing to write a PR for this once there is an agreement on the solution.

Thanks

React Native version: 0.61.2 -> master

Steps To Reproduce

  1. console.log() in JavaScript
  2. Look for the log in the Console.app or using log stream command

Describe what you expected to happen:
The logged text should appear in the Console.app

@LeoNatan
Copy link
Contributor Author

LeoNatan commented Jan 25, 2020

The log source input variable can be used to output native and JavaScript logs into different log categories, for better filtering capabilities.

https://developer.apple.com/documentation/os/1643744-os_log_create?language=objc

@PeteTheHeat
Copy link
Contributor

Migrating to Apple's unified os_log API seems like a great idea. It's iOS 10+, which won't be an issue at FB, as we've stopped supporting iOS9, is that also true of OSS?

My only concerns:

  • The privacy "feature" which considers dynamic strings and objects private. Will this be an issue? It seems everything we'd want to log is technically dynamic.
  • This appears to change the behaviour of what is being logged in production builds, correct? fprintf doesn't log in prod builds, while os_log will? My concern here is that some apps are logging a bunch of garbage which is currently stripped from prod builds, but now wouldn't be.

@LeoNatan
Copy link
Contributor Author

LeoNatan commented Jan 28, 2020

Thank you for your reply.

The public podspec is also iOS 10.0 and above:

s.platforms              = { :ios => "10.0", :tvos => "10.0" }

To answer your concerns:

If we set the default log level to debug, there would be no impact on performance.

In the code, I already see

#if RCT_DEBUG
static const RCTLogLevel RCTDefaultLogThreshold = (RCTLogLevel)(RCTLogLevelInfo - 1);
#else
static const RCTLogLevel RCTDefaultLogThreshold = RCTLogLevelError;
#endif

But I think this is unnecessary, or at least should be set to pass all RN logs to the system and let it decide what to do. As an example, Detox configures the logging system to capture debug and info messages; if a Detox user tests a release build of their app, the log entries should be allowed to reach the logging system, which will a. in case of a Detox test, collect all log entries, b. throw the non-error log entries on a production device.

@PeteTheHeat
Copy link
Contributor

Sounds great, thanks for the detailed explanation!

Is there anything else you'd want to know from our side? If not, you can go ahead and draft up a PR and I'll review/merge it.

@LeoNatan
Copy link
Contributor Author

Great, since we have an agreement, I'll write and submit a PR soon. Thanks

@LeoNatan
Copy link
Contributor Author

#27892

Thanks

@LeoNatan
Copy link
Contributor Author

Thank you!

osdnk pushed a commit to osdnk/react-native that referenced this issue Mar 9, 2020
Summary:
As discussed in facebook#27863, the following changes were made to modernize the internal default logging function:

- `RCTDefaultLogThreshold` is now set to `RCTLogLevelTrace` in both release and debug builds—the Apple logging system will discard uncollected log entires, while allowing for collection when needed
- `RCTLogLevel` is translated to the appropriate log type
- The log subsystem is "com.facebook.react.log"
- `RCTLogSource` translates to the appropriate category ("native"/"javascript")
- Log the provided message using `os_log_with_type`

Closes facebook#27863

## Changelog

[iOS] [Changed] - Use Apple unified logging API (os_log)
Pull Request resolved: facebook#27892

Test Plan:
## From Original PR

Ran a test app in the iOS simulator, and verified that logs are correctly displayed in Console.app as well as using the following command:
```sh
/usr/bin/xcrun simctl spawn booted log stream --level debug --style compact --predicate 'process=="ReactNativeTesterApp" && subsystem=="com.facebook.react.log"'
```

## Peter's Test Plan

1. Apply P125583473
2. Verify log output in Xcode P125583504
3. Apply this diff
4. Verify log output in Xcode P125583597

These appear unchanged, after digging into why, I realized that FB doesn't even use the default log function, we inject a custom one [here](https://fburl.com/diffusion/887a1axs). So this PR shouldn't affect us at all. :)

Differential Revision: D19605414

Pulled By: PeteTheHeat

fbshipit-source-id: 1d70fb702c337a759905d4a65a951a31353ce775
@facebook facebook locked as resolved and limited conversation to collaborators Oct 1, 2021
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants