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

[HttpClient] Adjust logger messages and levels #30873

Merged
merged 1 commit into from Apr 5, 2019

Conversation

Projects
None yet
4 participants
@lyrixx
Copy link
Member

lyrixx commented Apr 5, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 5, 2019

@joelwurtz

This comment has been minimized.

Copy link
Contributor

joelwurtz commented Apr 5, 2019

What about adding url / method / and other variables as attributes in the log message ? would be useful for some systems parsing logs so we could filters logs.

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Apr 5, 2019

What about adding url / method / and other variables as attributes in the log message ? would be useful for some systems parsing logs as we could some of this attributes to filters logs.

I already make app that heavily use an http client with a nice log platform, and I never add such need. So you really think it common to search by verb or URL ?
(Adding things to the context come with the a price: CPU (processing) / network / Storage. This is why I'm a bit reluctant to add it

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Apr 5, 2019

@nicolas-grekas About warning vs debug:

Warning do not spam the logs.

I suppose you are referring to the FingerCrossHandler. But the threshold is error.
debug < info < notice < warning < error < critical < alert < emerg
Ref

More over:

WARNING (300): Exceptional occurrences that are not errors. Examples: Use of deprecated APIs, poor use of an API, undesirable things that are not necessarily wrong.

So, maybe some warning should goes to debug. But I'm not sure it's the case for all of them.
For exemple the following case if legit to me

$this->logger && $this->logger->info(sprintf('Rejecting pushed response for "%s": authorization headers don\'t match the request', $url));

Could you re-review this PR with theses new considerations? Thanks

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 5, 2019

@lyrixx the HTTP/2 spec lists that clients should be defensive against servers that send too many pushes or other frames. That means this is not rare, when it's spamming.

@lyrixx lyrixx force-pushed the lyrixx:http-client branch from 2d0a8bf to 366d509 Apr 5, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Apr 5, 2019

@nicolas-grekas Ok. I changed all warning to debug, and updated all "%s" "%s" to "%s %s"

@lyrixx lyrixx force-pushed the lyrixx:http-client branch from 839fe6b to 04abc5c Apr 5, 2019

@nicolas-grekas nicolas-grekas force-pushed the lyrixx:http-client branch from 04abc5c to 098a7ac Apr 5, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 5, 2019

Thank you @lyrixx.

@nicolas-grekas nicolas-grekas merged commit 098a7ac into symfony:master Apr 5, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Apr 5, 2019

minor #30873 [HttpClient] Adjust logger messages and levels (lyrixx)
This PR was squashed before being merged into the 4.3-dev branch (closes #30873).

Discussion
----------

[HttpClient] Adjust logger messages and levels

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Commits
-------

098a7ac [HttpClient] Adjust logger messages and levels

@lyrixx lyrixx deleted the lyrixx:http-client branch Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.