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

logging: Add support for additional logger filters other than hostname #6082

Merged

Conversation

armadi1809
Copy link
Contributor

closes #5978

@francislavoie This took some time but here is my initial implementation of this. Would like to get your feedback on this and your thoughts on what other tests should go in here? Thanks.

@francislavoie
Copy link
Member

Like I said in the issue, I'm not totally convinced this is the right way to do this. But I can't really think of anything better.

I think no_hostname is better than no_host_name, because hostname is typically seen as a single word in the context web servers (see https://en.wikipedia.org/wiki/Hostname for example)

I think there should be a d.NextArg() when parsing that option to reject the config if an extra argument is passed, since it takes none right now.

I'm not sure if that's the best name though, I'd like @mholt's opinion.

@francislavoie francislavoie added the feature ⚙️ New feature or request label Feb 5, 2024
@francislavoie francislavoie added this to the v2.8.0 milestone Feb 5, 2024
caddyconfig/httpcaddyfile/builtins.go Show resolved Hide resolved
caddyconfig/httpcaddyfile/builtins.go Outdated Show resolved Hide resolved
modules/caddyhttp/logging.go Outdated Show resolved Hide resolved
modules/caddyhttp/logging.go Outdated Show resolved Hide resolved
@francislavoie francislavoie changed the title Logging: add support for additional logger filters other than hostname logging: Add support for additional logger filters other than hostname Feb 10, 2024
@armadi1809
Copy link
Contributor Author

@francislavoie, all the feedback items above should be resolved, except for the potential merge conflict with multiple hostnames.

@mholt
Copy link
Member

mholt commented Apr 15, 2024

@francislavoie @armadi1809 Let me know if you want to discuss anything to wrap up this PR. 👍

@armadi1809
Copy link
Contributor Author

If I remember correctly this needs to wait for #6088 to be merged as it will require some changes after that's done right? @francislavoie

@francislavoie
Copy link
Member

I'm merging it now (once CI passes) 👍 you can rebase your branch and make the necessary adjustments.

@armadi1809 armadi1809 force-pushed the log-filtering-enhancements branch 2 times, most recently from 90b3214 to 4f04d4c Compare April 21, 2024 03:47
@francislavoie
Copy link
Member

Needs another rebase, there's a conflict with another recent change of mine. Sorry!

@armadi1809
Copy link
Contributor Author

@francislavoie, rebase done. Any thoughts on the open discussion above?

caddyconfig/httpcaddyfile/builtins.go Outdated Show resolved Hide resolved
modules/caddyhttp/logging.go Outdated Show resolved Hide resolved
@francislavoie
Copy link
Member

I did a commit to do a final bit of cleanup and refactoring. I think this is good to go now.

@francislavoie francislavoie enabled auto-merge (squash) May 11, 2024 13:25
@francislavoie francislavoie merged commit 4356635 into caddyserver:master May 11, 2024
23 checks passed
@mholt
Copy link
Member

mholt commented May 11, 2024

Awesome, thank you both!!

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.

[RFE] filter/split access logs by fields other than hostname
3 participants