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

[iOS] Remove asl_log (Fixes #25380) #25382

Closed
wants to merge 2 commits into from

Conversation

JonnyBurger
Copy link
Contributor

Summary

By default absolutely everything gets logged twice (#25380).
This was introduced over 4 years ago here:

React Native Log to ASL · facebook/react-native@d1b14ef · GitHub

with the reason to support ASL.
With this PR the support for that will be removed but I believe this is justified because:

  • the benefit of not having every log message twice far outweighs the benefit of having Apple System Log.
  • ASL was superseded by "unified logging" (yellow box on https://developer.apple.com/documentation/os/logging).
  • I assume that people who use asl_log is very small. There is also very little information about it on the internet

Changelog

[iOS] [Fixed] - Logs would get printed twice

@pull-bot
Copy link

pull-bot commented Jun 24, 2019

Messages
📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 7531751

@react-native-bot react-native-bot added Platform: iOS iOS applications. Bug labels Jun 24, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 24, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Let's kill it.

@cpojer
Copy link
Contributor

cpojer commented Jun 25, 2019

@JonnyBurger it seems we have tests for this. Can you check out RCTLoggingTests and remove the associated tests?

@JonnyBurger
Copy link
Contributor Author

I don't see immediately which tests have to be changed D:

Running out of time this week, I asked @Kida007 if he can take a look tomorrow otherwise will try to get it merge ready next week!

@cpojer
Copy link
Contributor

cpojer commented Jun 28, 2019

I think this is actually a CI issue and not an issue with your change. Let me try to land it.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @JonnyBurger in 3b88f07.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jun 28, 2019
aleclarson pushed a commit to aleclarson/react-native-macos that referenced this pull request Jul 2, 2019
Summary:
By default absolutely everything gets logged twice (facebook/react-native#25380).
This was introduced over 4 years ago here:

[React Native Log to ASL · facebook/react-native@d1b14ef · GitHub](facebook/react-native@d1b14ef)

with the reason to support ASL.
With this PR the support for that will be removed but I believe this is justified because:

- the benefit of not having every log message twice far outweighs the benefit of having Apple System Log.
- ASL was superseded by "unified logging" (yellow box on https://developer.apple.com/documentation/os/logging).
- I assume that people who use asl_log is very small. There is also very little information about it on the internet

## Changelog

[iOS] [Fixed] - Logs would get printed twice
Pull Request resolved: facebook/react-native#25382

Differential Revision: D16048052

Pulled By: cpojer

fbshipit-source-id: 51519570bbee79571e4ff63f433b9b70bcd55671
aleclarson pushed a commit to aleclarson/react-native-macos that referenced this pull request Jul 2, 2019
Summary:
By default absolutely everything gets logged twice (facebook/react-native#25380).
This was introduced over 4 years ago here:

[React Native Log to ASL · facebook/react-native@d1b14ef · GitHub](facebook/react-native@d1b14ef)

with the reason to support ASL.
With this PR the support for that will be removed but I believe this is justified because:

- the benefit of not having every log message twice far outweighs the benefit of having Apple System Log.
- ASL was superseded by "unified logging" (yellow box on https://developer.apple.com/documentation/os/logging).
- I assume that people who use asl_log is very small. There is also very little information about it on the internet

## Changelog

[iOS] [Fixed] - Logs would get printed twice
Pull Request resolved: facebook/react-native#25382

Differential Revision: D16048052

Pulled By: cpojer

fbshipit-source-id: 51519570bbee79571e4ff63f433b9b70bcd55671
@aleclarson
Copy link
Contributor

aleclarson commented Jul 3, 2019

Hmmm, are we sure that os_log isn't necessary? I'm not seeing fprintf logs in Console.app, but it could just be a problem on my end.

I don't see any mention of fprintf (or stderr) on this page.

@JonnyBurger
Copy link
Contributor Author

Sorry for causing trouble.
Could we alternatively remove the other log statement instead?

So that it writes out and also prints to Console.app.

Maybe we can use 'unified logging' (see above) instead?

@aleclarson
Copy link
Contributor

IMO, we should ditch fprintf entirely for "unified logging" (eg: os_log), because it supports Console.app and log stream, and it stores logs in binary format (so less disk space, yet accessible in Release mode).

Here's an implementation of os_log in React Native, if you want to submit a PR:
https://gist.github.com/aleclarson/03e9b1eb6ed6c6fae8539d4500360bf5

@aleclarson
Copy link
Contributor

aleclarson commented Jul 4, 2019

The only issue is that it replicates the timestamp, so we should remove the timestamp argument from RCTFormatLog too.

Example:

2019-07-04 11:43:09.892 [info][tid:main][RCTCxxBridge.mm:216] Initializing <RCTCxxBridge: 0x101c01440> (parent: <LiteTimer.LTBridge: 0x600002908bd0>, executor: (null))
2019-07-04 11:43:09.892224-0400 LiteTimer[5928:31582] 2019-07-04 11:43:09.892 [info][tid:main][RCTCxxBridge.mm:216] Initializing <RCTCxxBridge: 0x101c01440> (parent: <LiteTimer.LTBridge: 0x600002908bd0>, executor: (null))

edit: To be less of a breaking change, maybe just pass nil to RCTFormatLog inside RCTDefaultLogFunction. 👍

@aleclarson
Copy link
Contributor

@cpojer Want me to send a PR for the two comments above?

@damandeepsingh93
Copy link

Now it doesnt Log in Console App. How to view logs in Release build now ?

@JonnyBurger
Copy link
Contributor Author

Sorry @damandeepsingh93 for breaking this.
Now I understand why this was there in the first place.

I think the correct behavior would then be to do asl_log in production and normal log in development?

M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
By default absolutely everything gets logged twice (facebook#25380).
This was introduced over 4 years ago here:

[React Native Log to ASL · facebook/react-native@d1b14ef · GitHub](facebook@d1b14ef)

with the reason to support ASL.
With this PR the support for that will be removed but I believe this is justified because:

- the benefit of not having every log message twice far outweighs the benefit of having Apple System Log.
- ASL was superseded by "unified logging" (yellow box on https://developer.apple.com/documentation/os/logging).
- I assume that people who use asl_log is very small. There is also very little information about it on the internet

## Changelog

[iOS] [Fixed] - Logs would get printed twice
Pull Request resolved: facebook#25382

Differential Revision: D16048052

Pulled By: cpojer

fbshipit-source-id: 51519570bbee79571e4ff63f433b9b70bcd55671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants