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

feat: add logger for use with tracing middleware #3509

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

lleadbet
Copy link
Contributor

In #3506, it was discussed to allow users to bring a custom logger to print out the various error logs that might happen during the tracing process to allow users to properly set the right structured logging consistently across the application.

This adds a new Logger attribute on the Tracer struct that can take in a Logger that abides by the standard log library standard (that is it has at a minimum has the Print/ln/f functions).

Defaults to a no-op logger.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented Jan 28, 2025

Coverage Status

coverage: 73.398% (-0.09%) from 73.492%
when pulling 19f442f on lleadbet:feat/log-hook-tracing
into 227a886 on 99designs:master.

Copy link
Collaborator

@StevenACoffman StevenACoffman left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the quick turn around. I have a few nits, but this looks great.

graphql/handler/apollofederatedtracingv1/logger/logger.go Outdated Show resolved Hide resolved
graphql/handler/apollofederatedtracingv1/logger/logger.go Outdated Show resolved Hide resolved
@lleadbet
Copy link
Contributor Author

Agreed on both points; had considered the package name (and New function) but was semi-concerned about folks importing that on accident thinking it was related to something else, not the tracing package, but given the import path including the tracing piece it was probably less important.

@StevenACoffman StevenACoffman merged commit 43712ca into 99designs:master Jan 28, 2025
17 checks passed
@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Jan 28, 2025

Thanks again for coming back to do this! I think this will make it a lot easier to debug if people have problems, and they won't be constrained to a specific logger.

I already cut a new release, so I'd like to wait a few days and accumulate some more stuff before making a new one.

@lleadbet
Copy link
Contributor Author

no rush on the new release, the actual needed feature was done and this is just a polishing piece :)

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.

3 participants