Skip to content

Log HTML rewriting errors #936

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

Merged
merged 1 commit into from
Aug 4, 2020
Merged

Conversation

Kixiron
Copy link
Member

@Kixiron Kixiron commented Aug 4, 2020

Logs any HTML rewriting errors to the console using log::error!() and increments a metric for failed html rewrites. Could be useful to see if we ever reach a point where we need to raise our memory limit

@Kixiron Kixiron added A-admin Area: Administration of the production docs.rs server A-backend Area: Webserver backend P-low Low priority issues labels Aug 4, 2020
@jyn514
Copy link
Member

jyn514 commented Aug 4, 2020

A few points:

  1. I don't expect to ever hit the memory limit in practice. From what @inikulin said in Switch from kuchiki to LOL_HTML #930 (comment), it seems like LOL uses basically no additional memory compared to storing the file in memory (❤️). However I'm ok with having this log just so we know that for a fact.
  2. The log should be in the web module, not in the rewrite itself, i.e. it should match on the value instead of moving globals into the rewriter. Otherwise we're making it even harder to eventually add benchmarks.
  3. This reasoning seems really strange to me:

It'd probably have roughly the same effect, but this makes sure that an error is always printed, regardless of if the error from the function is caught later on or not

Why should rewriting be special? If we want logging for requests that go wrong, we should add that directly instead of special-casing it here.

@Kixiron
Copy link
Member Author

Kixiron commented Aug 4, 2020

Addressed comments

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Looks good modulo nit :)

@jyn514 jyn514 merged commit acc0f06 into rust-lang:master Aug 4, 2020
@Kixiron Kixiron deleted the oom-reporting branch August 4, 2020 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-admin Area: Administration of the production docs.rs server A-backend Area: Webserver backend P-low Low priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants