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

feat: log CONNECT error response body #139

Merged
merged 3 commits into from
May 22, 2024
Merged

Conversation

barjin
Copy link
Contributor

@barjin barjin commented May 14, 2024

Connected to https://github.com/apify/apify-proxy/issues/823.

On an internal proxy error (such as trying to access an invalid or unresolvable URL - e.g. https://example.comundefined), the proxy returns a response with the error message. got-scraping only logs the length of the message, which is quite useless for most applications.

@barjin barjin requested review from janbuchar and vladfrangu May 14, 2024 08:42
@barjin barjin self-assigned this May 14, 2024
@github-actions github-actions bot added this to the 89th sprint - Tooling team milestone May 14, 2024
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label May 14, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Pull Request Tookit has failed!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

@barjin barjin added the adhoc Ad-hoc unplanned task added during the sprint. label May 14, 2024
Comment on lines 47 to 49
Below is the first 100 bytes of the proxy response body:
${head.toString('utf8', 0, 100)}
`));

Choose a reason for hiding this comment

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

Seems weird to include this kind of formatting in the message of an Error, but we probably don't have any other options, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could squish it in cause, but the intuition (and MDN doc) tells me that the message is for human readable errors and cause is for structured data / original errors in case of rethrowing.

Choose a reason for hiding this comment

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

Yeah, sounds reasonable. Still, the newlines freak me out 😄

Could we also include the whole response body somewhere in cause?

Copy link
Member

Choose a reason for hiding this comment

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

multiline error messages are quite common, we use those in several places already, also playwright and puppeteer do the same (and much worse, e.g. 10+ lines error messages, namely those about a missing browser executable). you can attach metadata to the error instance too, but its better to have a custom error class for that so its discoverable. we also use this approach (but we don't add any metadata there, just custom error prototypes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to bump got to have sindresorhus/got#2327 , which caused the build to break... fixed by the tsconfig.json update. Happy to get feedback on that, it fixes the build, but might not be the best way around it.

@barjin barjin merged commit a8a5dfe into master May 22, 2024
8 checks passed
@barjin barjin deleted the feat/log-proxy-error-body branch May 22, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants