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

fix: reduce log level for matching to debug #454

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

mefellows
Copy link
Member

Screenshot 2024-07-05 at 12 03 58 AM

The additional noise in the logs at info level is not needed and interferes with standard out (e.g. test output), and is more appropriate for debug level.

@YOU54F
Copy link
Member

YOU54F commented Jul 4, 2024

some other places noted from pact-foundation/pact-js#1114

it’s here

https://github.com/pact-foundation/pact-core-mock-server/blob/febf7974b648aeeefb6af3d69dd988051886243e/pact_mock_server/src/hyper_server.rs#L275

here

https://github.com/pact-foundation/pact-core-mock-server/blob/febf7974b648aeeefb6af3d69dd988051886243e/pact_mock_server/src/hyper_server.rs#L185

The debug values captured for the incoming and outgoing requests are useful and could be captured for sending in case of failure.

and here

info!("comparing to expected {}", expected);

There are other instances of info! statements

I think lib authors can suppress log output by piping to a buffer (or file) and reading from there if necessary (fetch_log_buffer_as_json)

@YOU54F YOU54F self-requested a review July 4, 2024 15:44
Copy link
Member

@YOU54F YOU54F left a comment

Choose a reason for hiding this comment

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

Could capture additional info statements found here, in this PR if you wanted to, although they cross crates

https://github.com/search?q=repo%3Apact-foundation%2Fpact-reference+info%21&type=code

@rholshausen rholshausen merged commit 42c083e into master Jul 4, 2024
62 of 63 checks passed
@rholshausen rholshausen deleted the chore/reduce-info-logs-from-hyper-matches branch July 4, 2024 23:04
@mefellows
Copy link
Member Author

Could capture additional info statements found here, in this PR if you wanted to, although they cross crates

https://github.com/search?q=repo%3Apact-foundation%2Fpact-reference+info%21&type=code

Good shout, they look like decent targets to switch to debug! as well.

Pact JS was one of the earlier adopters and we were toying with various output options. Resolving pact-foundation/pact-js-core#428 will go some way to making this less relevant in the JS use case.

@rholshausen
Copy link
Contributor

The other option is if you don't want to see INFO level logs, then don't set the logging to INFO. Set it to WARN.

@YOU54F
Copy link
Member

YOU54F commented Jul 5, 2024

this is pretty much ready to go pact-foundation/pact-js-core#428

however it is a pain to test as when the ffi lib is started and you set a log sink, you can't change it, so it logs to stdout/buffer or file. it's the cause of noise in the pact-go and js logs about the ffi already being initialised and a log level is already set, so cannot set a new level

Not sure if we can setup multiple?

Also it would actually be nice if we could control the log level per test, but that currently isn't possibly. I would find maybe being able to log per test more useful that a massive log dump in a file.

makes the haystack smaller if something does go wrong, as no-one wants to go sifting through logs unless there is a really good key that you can reliably search on.

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