8000 [HttpClient] Adjust logger messages and levels by lyrixx · Pull Request #30873 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpClient] Adjust logger messages and levels #30873

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 5, 2019
Merged

[HttpClient] Adjust logger messages and levels #30873

merged 1 commit into from
Apr 5, 2019

Conversation

lyrixx
Copy link
Member
@lyrixx 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

@lyrixx lyrixx mentioned this pull request Apr 5, 2019
Merged
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 5, 2019
@joelwurtz
Copy link
Contributor
joelwurtz commented Apr 5, 2019 •
edited
Loading

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
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
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
Copy link
Member

@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
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"

@nicolas-grekas
Copy link
Member

Thank you @lyrixx.

@nicolas-grekas nicolas-grekas merged commit 098a7ac into symfony:master Apr 5, 2019
nicolas-grekas added a commit that referenced this pull request Apr 5, 2019
@nicolas-grekas
minor #30873 [HttpClient] Adjust logger messages and levels (lyrixx)
8977f74 
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 http-client branch April 5, 2019 14:22
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
HttpClient Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
5 participants
@lyrixx @joelwurtz @nicolas-grekas @javiereguiluz @carsonbot
0