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

fastcgi: allow users to log stderr output (#4967) #5004

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

flga
Copy link
Collaborator

@flga flga commented Sep 1, 2022

Following the discussion in #4967 I've added support for logging any stderr messages sent by the upstream.

It is off by default and can be enabled with:

transport fastcgi {
    capture_stderr
}

Naming it capture gives us room to not buffer stderr records if it's off but it also isn't super clear that the main purpose is logging (at least right now). On the other hand, I don't know what else could be done with this besides logging. We should think whether this name makes sense.

Logging is done through the FastCGI transport logger.
Messages received on stderr are logged at WARN level by default, or ERROR level if the response status is >= 400.

The field name body was chosen instead of message because there's not really a clear definition of what a message is, there is no way for us to know, from a given stderr response, where one begins and another one ends. We cannot rely on fastcgi record boundaries alone (at least with PHP).

The output is messy but that's not something we can address, and this behaviour is pretty much equivalent to apache/nginx with the added bonus that formatting is preserved (albeit escaped) and we do not truncate the contents.

The logging in the fastcgi transport now also complies with log_credentials (also applies to logging of fastcgi params).
I'm not sure that we should be logging the fastcgi params though. It's somewhat redundant with the request data, but the alternative is telling users to do "printf debugging". I can see both sides so I defaulted to logging them, let me know what you think.

The changes could be a little cleaner (especially the handling of transfer encoding) but given that there's a PR to overhaul fastcgi as a whole I tried to keep the changeset as small as possible so that the merge is more obvious.
On that note, a test couldn't hurt, but I don't see how to output stderr records from a net/http/fcgi handler (what we use in the tests) so we'd pretty much just be asserting that the zap logger is called. Doesn't seem super worth it, especially since the overhaul is in progress, but since this got lost in the v1 -> v2 transition it's a good argument for doing it. Let me know what you think.

One thing I could not avoid were the comment formatting changes included in 1.19's gofmt.
I tried to fight it but it's painful. If this causes problems with our own parsing (for docs) let me know and I'll fix it up by hand.

@CLAassistant
Copy link

CLAassistant commented Sep 1, 2022

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie added the feature ⚙️ New feature or request label Sep 1, 2022
@francislavoie francislavoie added this to the v2.6.0-beta.1 milestone Sep 1, 2022
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Hey this is pretty good @flga -- thank you for contributing it. I look forward to @WeidiDeng's comments as well since he has a conflicting branch up to optimize the performance of this transport.

One thing I could not avoid were the comment formatting changes included in 1.19's gofmt.

Yeah, I noticed that too on a bunch of my other commits lately. I decided to just roll with it and hope it doesn't break anything.

Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Tried this out, works great!

Thanks for making the executive decision for me with implementing this as an opt-in 😅

@francislavoie francislavoie merged commit f2a7e7c into caddyserver:master Sep 2, 2022
@mholt
Copy link
Member

mholt commented Sep 2, 2022

@flga We both think this was a really high-quality proposal and change. We're going to send you an invite to be a collaborator, meaning you'll have push rights to the repo so in the future you can easily make patches on branches rather than forks. And you can participate in other ways as you would like. 🙂 Thank you again for contributing!

@flga flga deleted the fastcgi-stderr branch September 4, 2022 15:20
@flga
Copy link
Collaborator Author

flga commented Sep 4, 2022

@mholt Thanks! That was a little unexpected, cheers 😁

@mholt mholt modified the milestones: v2.6.0-beta.1, v2.6.0 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants