-
Couldn't load subscription status.
- Fork 45
Provide log format options #463
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
base: develop
Are you sure you want to change the base?
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: 1b239b85ab6b3404101b79f7 URL: https://www.apollographql.com/docs/deploy-preview/1b239b85ab6b3404101b79f7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized this is still in draft. Feel free to ignore most of these!
| use tracing_subscriber::EnvFilter; | ||
| use tracing_subscriber::fmt::Layer; | ||
| use tracing_subscriber::fmt::writer::BoxMakeWriter; | ||
| use tracing_subscriber::{EnvFilter, Layer as LayerTrait, Registry}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This should be split up like the other imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify why?
768b10c to
87886f7
Compare
Co-authored-by: Nicholas Cioli <[email protected]>
Co-authored-by: Nicholas Cioli <[email protected]>
| .with_writer(writer) | ||
| .with_ansi(self.ansi_enabled) | ||
| .with_target(false) | ||
| .boxed(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicholascioli is there a cleaner way of doing this? Since full, compact, json, and pretty all return a different type I could not figure out how to return from this match block without erasing the type (calling boxed). Let me know if there is a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Just small suggestions.
| impl Logging { | ||
| pub fn env_filter(logging: &Logging) -> Result<EnvFilter, anyhow::Error> { | ||
| let mut env_filter = EnvFilter::from_default_env().add_directive(logging.level.into()); | ||
| pub struct LoggingLayerBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can derive Default for LoggingLayerBuilder instead of manually implementing it, which is more idiomatic:
If we add it here,
| pub struct LoggingLayerBuilder { | |
| #[derive(Default)] | |
| pub struct LoggingLayerBuilder { |
L69 will look like this:
impl LoggingLayerBuilder {
pub fn new() -> Self {
Self::default()
}
}| // This primarily to be used by unit tests to allow for dependency injection. | ||
| // If no writer is provided when build() is called, one will be created using the logging config options. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding production code just for tests isn't ideal. How about only compiling these methods during tests?
| // This primarily to be used by unit tests to allow for dependency injection. | |
| // If no writer is provided when build() is called, one will be created using the logging config options. | |
| #[allow(dead_code)] | |
| #[cfg(test)] |
Adding configuration options for logging output format. The available options are
full(default),compact,json, andpretty.