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] Debugging option #34289

Open
B-Galati opened this issue Nov 8, 2019 · 10 comments
Open

[HttpClient] Debugging option #34289

B-Galati opened this issue Nov 8, 2019 · 10 comments

Comments

@B-Galati
Copy link
Contributor

@B-Galati B-Galati commented Nov 8, 2019

Description
At the moment it's quite hard to debug HTTP errors because the only information we get is:

HTTP/1.1 400 Bad Request returned for "https://example.com/".

It would be nice to have an opt-in option that would automatically open response payload and headers in order to log them in Sentry for example. It may require to handle a max lenght on the response payload.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 8, 2019

4.4 has a profiler panel, and $response->getInfo('debug') gives the full log.
What else do you mean?

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 8, 2019

log them in Sentry for example

hum, I missed this part - so you mean a monolog processor.

PR welcome in the Monolog Bridge I suppose.

@B-Galati
Copy link
Contributor Author

@B-Galati B-Galati commented Nov 8, 2019

I am also talking about production debugging.

I would like the thrown exception to contain the response body / headers / etc. if the client has been declared with a specific option for example.

For example instead of having HTTP/1.1 400 Bad Request returned for "https://example.com/". in log we would have HTTP/1.1 400 Bad Request returned for "https://example.com/". <payload> <headers>.

A monolog processor could do the job indeed. Is it better than a new option on the HttpClient in your opinion?

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 8, 2019

A monolog processor could do the job indeed. Is it better than a new option on the HttpClient in your opinion?

totally - that's not a concern the HttpClient needs to care about.

@B-Galati
Copy link
Contributor Author

@B-Galati B-Galati commented Jan 9, 2020

If it can help I did something like this in a monolog processor

    $exception = $record['context']['exception'] ?? null;
        if ($exception instanceof \Exception && $record['message'] !== $exception->getMessage()) {
            $record['message'] = sprintf('%s - %s', $record['message'], $exception->getMessage());
        }
        while ($exception instanceof \Throwable) {
            if ($exception instanceof HttpExceptionInterface) {
                // It needs to be the 1st statement in order to fullfil the request
                $responseContent = mb_strimwidth($exception->getResponse()->getContent(false), 0, 1000);
                $record['context']['http_client'][] = $exception->getResponse()->getInfo()
                    + ['response_content' => $responseContent];
            }
            $exception = $exception->getPrevious();
        }
@stof
Copy link
Member

@stof stof commented Jan 9, 2020

the drawback of reading the response content there is that your logger will now wait until the full body is received. that's why we don't do it in the core.

@B-Galati
Copy link
Contributor Author

@B-Galati B-Galati commented Jan 9, 2020

@stof Yes, that's why I combine this with a crossfinger + buffer handler to limitate this problem.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 2, 2020

TraceableHttpClient now can collect response bodies without breaking async. I'd suggest having a look to implement this. Anyone up for giving it a try? @B-Galati?

@B-Galati
Copy link
Contributor Author

@B-Galati B-Galati commented Feb 2, 2020

@nicolas-grekas Awesome! Which PR enables this? I'll give it a try this week.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 2, 2020

Check #35407

B-Galati added a commit to B-Galati/symfony_reproducer_34289 that referenced this issue Feb 11, 2020
B-Galati added a commit to B-Galati/symfony_reproducer_34289 that referenced this issue Feb 11, 2020
B-Galati added a commit to B-Galati/symfony_reproducer_34289 that referenced this issue Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.