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: Customizable zapcore.Core #6381

Merged
merged 1 commit into from
Jun 10, 2024
Merged

logging: Customizable zapcore.Core #6381

merged 1 commit into from
Jun 10, 2024

Conversation

kkroo
Copy link
Contributor

@kkroo kkroo commented Jun 8, 2024

This change enables support custom caddy.logging.cores that implement zapcore.Core for a sentry event reporting plugin that reports errors and their traces at github.com/kkroo/caddy-sentry

@mholt
Copy link
Member

mholt commented Jun 8, 2024

Thanks for the contribution!

Since we're teeing, I wonder if multiple cores (a slice of cores) would be worthwhile? Maybe not. Just thinking.

If not, this looks pretty good and we can probably merge it after a quick review. (I think godoc comments will be needed)

@francislavoie
Copy link
Member

I think an output module is still always needed, right? So I guess you'd want to use output discard along with this maybe, so you're not required to also write to a file or stderr if you want to send it to a module like Sentry? That's my main concern about tee-ing the core.

This also needs Caddyfile support I think, and it would be nice if we could have somekind of mock core module that we can use for testing configuration etc.

@francislavoie francislavoie changed the title Customizable logging zapcore.Core logging: Customizable zapcore.Core Jun 8, 2024
@francislavoie francislavoie added the feature ⚙️ New feature or request label Jun 8, 2024
@francislavoie francislavoie added this to the v2.9.0 milestone Jun 8, 2024
@kkroo
Copy link
Contributor Author

kkroo commented Jun 9, 2024

Thanks for the feedback, addressing comments .

@mholt An implementation of this interface could chain up multiple cores if needed. I can add the doc strings.

@francislavoie It uses the default logging core. If the user doesn't want additional output output discard

@kkroo kkroo force-pushed the zapcore branch 2 times, most recently from 39bd590 to 50fa1a6 Compare June 9, 2024 05:25
logging.go Outdated Show resolved Hide resolved
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.

This LGTM now :) Thank you!

@mholt mholt merged commit d85cc2e into caddyserver:master Jun 10, 2024
23 checks passed
@kkroo kkroo deleted the zapcore branch June 10, 2024 19:36
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.

3 participants