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

Prints server logs to "stderr" #326

Merged
merged 2 commits into from
Jun 16, 2021
Merged

Conversation

nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Jun 15, 2021

This pull request addresses #320, #325, by proxy applications logs (using stderr driver) to stderr.

Screenshot 2021-06-15 at 14 22 46

@Radiergummi
Copy link

Can confirm this works well for Roadrunner, in both production and local environments. I'll test Swoole later today.

@nunomaduro
Copy link
Member Author

@Radiergummi Please try to also test in some edge cases, to ensure this change won't effect with users.

Copy link
Contributor

@Namoshek Namoshek left a comment

Choose a reason for hiding this comment

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

I only have a very simple application to test this changes right now, so I cannot really go into feature test details or anything.


The solution does what the stderr logger suggests - it logs server messages on stderr. It does this for all server messages though, no matter if they come from stdout or stderr. It also does not respect the actual logger config of the application. I'm not sure if it should though (because I've no idea what stuff RR and swoole print besides the application logs).

An ideal solution, in my opinion, would only forward stderr output of the worker processes to stderr of the main process. When reading the RR documentation, I though this could be achieved with logs.err_output setting, but it does not seem to do what the name suggests. So maybe this isn't even possible.

src/Commands/Concerns/InteractsWithIO.php Show resolved Hide resolved
src/Commands/StartRoadRunnerCommand.php Show resolved Hide resolved
@nunomaduro
Copy link
Member Author

@Namoshek Your last comment was not clear for me. At this time, only server messages - outputted by the Laravel application - are captured and proxy to the stderr by Octane.

@Namoshek
Copy link
Contributor

@nunomaduro Exactly, everything from the server channel. What I was aiming at: for me is unclear whether the server channel contains only application logs (from the stderr logger) or also other logs/messages which I could not trigger in my very basic setup. If the latter is the case, we probably need some additional checks besides the ignored messages you already added. But I'm fine with adding those as they come up.

The only thing which is still a bit weird to me: why are stderr logger logs not on the stderr output of the worker process? Only forwarding stderr -> stderr and not stdout -> stderr would be the best solution after all, or am I missing something?

@nunomaduro
Copy link
Member Author

@nunomaduro Exactly, everything from the server channel. What I was aiming at: for me is unclear whether the server channel contains only application logs (from the stderr logger) or also other logs/messages which I could not trigger in my very basic setup. If the latter is the case, we probably need some additional checks besides the ignored messages you already added. But I'm fine with adding those as they come up.

Agree.

The only thing which is still a bit weird to me: why are stderr logger logs not on the stderr output of the worker process? Only forwarding stderr -> stderr and not stdout -> stderr would be the best solution after all, or am I missing something?

That seems to be a roadrunner thing. We actually don't even touch on that configuration, so I would also expect those to be in the stderr.

Screenshot 2021-06-15 at 19 30 14

@Radiergummi
Copy link

An ideal solution, in my opinion, would only forward stderr output of the worker processes to stderr of the main process. When reading the RR documentation, I though this could be achieved with logs.err_output setting, but it does not seem to do what the name suggests. So maybe this isn't even possible.

As far as I know from a Spiral project, the err_output actually refers to error messages emitted by the roadrunner process, but it doesn't relate to the PHP application hosted by Roadrunner. It uses stdout to emit HTTP responses, so the only side channel left for logs is stderr.

@Radiergummi Please try to also test in some edge cases, to ensure this change won't effect with users.

I monkey-patched our main production workload with the PR changes, compared the output to our php-fpm variant, and did not notice any differences in log output. Printing huge messages (str_repeat('x', 9_000_000)) worked fine, as did stack traces from the exception handler including line breaks. As far as I can see, this feature works as intended and restores the ability to write logs in roadrunner deployments.

Concerning the server channel: It would be preferable if Roadrunner had a log channel exclusive to the hosted application, or alternatively sent unrelated messages to another channel. Maybe they'd be open for such a change?

@nunomaduro
Copy link
Member Author

Concerning the server channel: It would be preferable if Roadrunner had a log channel exclusive to the hosted application, or alternatively sent unrelated messages to another channel. Maybe they'd be open for such a change?

Not sure - can you ask them? Meanwhile, making this PR open for review.

@nunomaduro nunomaduro marked this pull request as ready for review June 16, 2021 08:10
@Radiergummi
Copy link

Not sure - can you ask them? Meanwhile, making this PR open for review.

Yep, I planned to open a Roadrunner issue on this. But if that is implemented at all, it's going to take a while, so it's out of scope for this PR anyway.

@taylorotwell taylorotwell merged commit bb5b628 into 1.x Jun 16, 2021
@nunomaduro nunomaduro deleted the feat/proxy-raw-output-to-stderr branch June 16, 2021 13:38
@Radiergummi
Copy link

@nunomaduro is there an ETA on the next release? I'm just now trying to debug an issue in production, but I don't have any stack traces in the application logs...

@driesvints
Copy link
Member

@Radiergummi releases are done on Tuesday's.

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.

None yet

5 participants