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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

logging: Automatic wrap default for filter encoder #5980

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

francislavoie
Copy link
Member

I was talking with @dunglas and he pointed out how awkward it is to have to configure wrap on the filter encoder because it loses the nice-to-have default behaviour of checking if stderr is a terminal to choose a default of console vs json.

Getting this to work just right was quite messy, but I think I got it working well now. I also made some adjustments that may or may not be considered breaking changes for some people, since we're changing some default behaviour for the filter encoder, and I slightly changed the logic for detecting the terminal.

  • we're now checking if stderr is a terminal instead of stdout because our default writer is stderr anyway, it was kinda incorrect to be checking stdout I think
  • fixed a bug where the writer wasn't considered when determining the default encoder; this meant that if a output file foo.log was configured and it was run at a terminal, it would write console logs to the file by default (but with color turned off). That's a bad default, it should be writing json to the file even when run as a terminal unless otherwise stated.
  • now we're never removing color by default from the log output. What we would need to do is detect whether the terminal supports color or not, but we weren't doing that before so this is certainly not a regression, if anything it's just a gap, but it's not the end of the world. I do know some Windows users have complained when they used the legacy command prompt which didn't support unix style colors, but the new windows terminal does support it now so 馃し I don't really care to deal with that detail right now, not a big deal
  • wrap is no longer required for filter encoder, it now defaults to json when stderr is not a terminal, and console when it is a terminal
  • to make this work, we now pass down the logger's writer module to the configured encoder if it has the ConfiguresFormatterDefault interface, which is called immediately after provisioning as a 2nd phase in provisioning
  • the filter encoder implements that interface, and uses it to check if the writer is a terminal, switching the nested encoder to console; it also passes down the writer if the explicitly configured encoder if itself wants the writer (e.g. filter inside filter, or whatever).

@francislavoie francislavoie added bug 馃悶 Something isn't working feature 鈿欙笍 New feature or request labels Dec 14, 2023
@francislavoie francislavoie added this to the v2.8.0 milestone Dec 14, 2023
Copy link
Collaborator

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Thank you very much Francis! This will help improving Mercure default config.

logging.go Outdated Show resolved Hide resolved
modules/logging/filterencoder.go Outdated Show resolved Hide resolved
modules/logging/filterencoder.go Outdated Show resolved Hide resolved
@mholt
Copy link
Member

mholt commented Jan 24, 2024

Thanks for working on this! Yuck 馃槣

Likely to accept, but first, @dunglas Can you help me see the config difference for Mercure, before and after this change? I'm mainly curious so I can understand the impact here.

@francislavoie
Copy link
Member Author

Ah right, I'll add an adapt test to show the difference.

@dunglas
Copy link
Collaborator

dunglas commented Jan 24, 2024

Currently: https://github.com/dunglas/mercure/pull/862/files

Hopefully, after, the same config in both files, automatically adapting to console or json depending on if run in TTY mode or not:

	log {
		format filter {
			fields {
				uri query {
					replace authorization REDACTED
				}
			}
		}
	}

@mholt
Copy link
Member

mholt commented Jan 25, 2024

Gotcha, so it allows you to remove the wrap line it looks like

@francislavoie
Copy link
Member Author

Correct.

francislavoie and others added 5 commits January 24, 2024 22:50
Co-authored-by: K茅vin Dunglas <kevin@dunglas.fr>
Co-authored-by: K茅vin Dunglas <kevin@dunglas.fr>
Co-authored-by: K茅vin Dunglas <kevin@dunglas.fr>
@francislavoie
Copy link
Member Author

Added a test, ready to go IMO.

@francislavoie francislavoie enabled auto-merge (squash) January 25, 2024 03:57
@francislavoie francislavoie merged commit b9c40e7 into master Jan 25, 2024
25 checks passed
@francislavoie francislavoie deleted the smart-filter-wrapped-default branch January 25, 2024 04:00
@francislavoie
Copy link
Member Author

Oh lmao, @dunglas' review from last month was enough to allow it to get merged 馃槀 I didn't expect that a review from before he was granted collaborator status would be valid for that. Fun detail of github I guess!

@mholt
Copy link
Member

mholt commented Jan 25, 2024

ohhh, lol, that's fun -- so reviews are retroactive in granting merge permissions. That's neat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 馃悶 Something isn't working feature 鈿欙笍 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants