[Hidden] Display a friendly message when a background report times out #10644

mkllnk · 2023-04-03T07:25:50Z

What? Why?

First step towards Make previously generated reports available for download #10280

What should we test?

Activate background reports.
Visit /admin/reports
Choose a complex report.
Choose parameters to make it time out. uk-staging orders_and_distributors since 2021 should time out but not run too long.

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

jibees

Clear. Perhaps it's worth having a screenshot in the PR description for instance managers, just to get an idea of the design?

mkllnk · 2023-04-03T08:09:43Z

Thanks for the quick review. I just discovered that that the dev environment is broken now without the timeout gem. Back to dev.

The closer the test environment is to the production environment the more realistic the tests will be, and the more code we test. We are now able to test the app behaviour on timeouts which I want to do for reports. We can also catch incompatibilities with the rack-timeout gem during testing.

Once we get a download link for a report, we can display this message sooner. But for now we just use the existing request timeout.

dacook

Great work!

dacook · 2023-04-04T03:31:25Z

app/controllers/admin/reports_controller.rb

+
+    def render_timeout_error
+      assign_view_data
+      @error = ".report_taking_longer"


I'm curious if there's any reason to translate in the view rather than here? I think we can access the translate helper in the controller.

This looks nice and clean, but I was thinking we usually expect translate keys to be with the t() method call, so if we ever need to rely on that pattern (eg when searching the codebase or perform mass find/replace), this could slow us down or cause us to miss it.

It's a really minor point but I was curious :)

I'm curious if there's any reason to translate in the view rather than here?

Yes, Rails supports translations in the view better.

We can use the lazy loading with the prefixed dot.

We can simply use t instead of I18n.t.

Views are concerned with how things are displayed. An error code can be translated or a different error code could trigger a modal and that should be the concern of the view, not the controller, I think.

You got a good point about the disassociation of translation key and the t call as well. And I'm not sure what to suggest about that. If we want to translate in the view then we could do something like t(".report_taking_longer") if @error == :report_taking_longer but that seems cumbersome, too. So we can just translate in the controller and add an apt translation scope. My pattern is a bit closer to the translations of errors on models. There's a default structure of error messages on ActiveRecord errors. Maybe the path will become more clear later.

Good points.

dacook · 2023-04-04T03:33:27Z

config/locales/en.yml

@@ -1438,6 +1438,11 @@ en:
      pack_by_customer: Pack By Customer
      pack_by_supplier: Pack By Supplier
      pack_by_product: Pack By Product
+      show:
+        report_taking_longer: >
+          This report is taking longer to process.


This seems to imply that it's still running, and leaves you unsure if you'll receive any results or not. How about:

Suggested change

This report is taking longer to process.

Sorry, this report took too long to process.

In fact, it is still running when background reports are enabled. I haven't implemented the timeout yet. But the user also doesn't get a result yet. So it's confusing. And without background reports, it was cancelled. I'll take your suggestion but it will be changed in my next pull request which will allow to download the report.

dacook · 2023-04-04T03:36:21Z

spec/system/admin/reports_spec.rb

+      expect_any_instance_of(Admin::ReportsController).to receive(:sleep).
+        and_raise(Rack::Timeout::RequestTimeoutException.new(nil))


👍 This is a nice quick and direct method to test for a timeout.
It's not as real as simulating a real timeout, but at least it doesn't slow down our test runs unnecessarily.

This functionality is also relevant for non-background processing. I'm guessing that's less practical to test though.
Is it worth testing in a controller spec though?

I never really know where to draw the line. This seems like good enough coverage.

I thought of different ways of using Timecop or reduce the timeout to a second but they all don't work with rack-timeout because:

Rack::Timeout reads the configuration at boot time and you can't modify it later unless you dig into the guts of it and mock private methods which would be brittle.

Rack::Timeout uses it's own monotonic timer as far as I could gather from looking at the source. So modifying the system's time doesn't help.

So if we don't want to wait 15 seconds for the timeout, we do have to raise the error ourselves.

This functionality is also relevant for non-background processing. I'm guessing that's less practical to test though.

We would need to mock another part to raise an exception. Possible but I don't think it's worth it at the moment. The old behaviour is that the snail is displayed on a timeout. The same will happen if my code is buggy, just with a different response code. Low risk here. And this is only temporary. Soon we want to stop the waiting at 10 seconds, before the Rack timeout,

dacook

Oops, that was meant to be a suggestion, not approval. Can you please have a look at my suggested change and apply if you agree?

filipefurtad0 · 2023-04-05T17:53:41Z

Hey @mkllnk ,

After staging this PR but before enabling the toggle, running a large report (over two years, as superadmin), triggers a 404:

After enabling the feature background_reports, I get:

I got this same result after disabling sidekiq as you've indicated, and running the report again.

So, it does not look like the message is being rendered to the user. Any idea what to try next? Or should it be moved back into In Dev?

mkllnk · 2023-04-11T05:15:56Z

Hm, it works in development but not in staging. The difference is nginx. I think that the error message you see comes from nginx which has the same timeout. My error message is probably coming half a second too late.

Options:

Increase the nginx timeout.
Reduce the report timeout.

Since the long-term plan is to reduce the report timeout to ten seconds and then have a downloadable link, I should just work on that and submit a bigger pull request. Maybe annoying for reviewers but better for testing. 😄

I'll probably close this one in favour of the next PR with download link.

This allows us to display a friendly message before nginx displays its default error.

mkllnk · 2023-04-11T07:11:12Z

I chose a quicker option. Just add a shorter timeout (two seconds less) on this branch and we can reduce that later. This can be tested again. The timeout is only reduced and the message is only shown when background reports are activated.

filipefurtad0 · 2023-04-11T18:40:35Z

Cool - Thanks @mkllnk!

It's working now 🎉

With the toggle activated, it took about 45 secs:

Without the toggle activated, it took about almost two minutes:

Merging 👍

mkllnk · 2023-04-11T23:22:14Z

With the toggle activated, it took about 45 secs:

That's odd. It should only be two seconds quicker than without the toggle. And without the toggle it should take two minutes, maybe off by a few seconds. Let's see what further testing reveals.

mkllnk self-assigned this Apr 3, 2023

mkllnk marked this pull request as ready for review April 3, 2023 07:28

mkllnk mentioned this pull request Apr 3, 2023

Make previously generated reports available for download #10629

Closed

jibees approved these changes Apr 3, 2023

View reviewed changes

mkllnk marked this pull request as draft April 3, 2023 08:09

mkllnk added 2 commits April 4, 2023 11:08

Display a friendly message when a report times out

00a3976

Once we get a download link for a report, we can display this message sooner. But for now we just use the existing request timeout.

mkllnk force-pushed the report-timeout-message branch from b4f9800 to 00a3976 Compare April 4, 2023 01:08

mkllnk marked this pull request as ready for review April 4, 2023 01:11

dacook approved these changes Apr 4, 2023

View reviewed changes

dacook requested changes Apr 4, 2023

View reviewed changes

Explain report timeout better to user

020af0c

mkllnk force-pushed the report-timeout-message branch from c352048 to 020af0c Compare April 4, 2023 23:59

mkllnk requested a review from dacook April 5, 2023 00:10

dacook approved these changes Apr 5, 2023

View reviewed changes

mkllnk mentioned this pull request Apr 5, 2023

[Hidden] Provide download link for reports generated in the background #10661

Merged

filipefurtad0 self-assigned this Apr 5, 2023

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Apr 5, 2023

filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Apr 5, 2023

mkllnk closed this Apr 11, 2023

Background reports time out earlier than nginx

30184ac

This allows us to display a friendly message before nginx displays its default error.

mkllnk reopened this Apr 11, 2023

mkllnk changed the title ~~Display a friendly message when a report times out~~ [Hidden] Display a friendly message when a report times out Apr 11, 2023

mkllnk changed the title ~~[Hidden] Display a friendly message when a report times out~~ [Hidden] Display a friendly message when a background report times out Apr 11, 2023

mkllnk added pr-staged-uk staging.openfoodnetwork.org.uk and removed feedback-needed labels Apr 11, 2023

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Apr 11, 2023

filipefurtad0 merged commit 0971e8d into openfoodfoundation:master Apr 11, 2023

mkllnk deleted the report-timeout-message branch April 11, 2023 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hidden] Display a friendly message when a background report times out #10644

[Hidden] Display a friendly message when a background report times out #10644

mkllnk commented Apr 3, 2023 •

edited

Loading

jibees left a comment

mkllnk commented Apr 3, 2023

dacook left a comment

dacook Apr 4, 2023

mkllnk Apr 4, 2023

dacook Apr 5, 2023

dacook Apr 4, 2023

mkllnk Apr 4, 2023

dacook Apr 4, 2023

dacook Apr 4, 2023

mkllnk Apr 4, 2023

dacook left a comment

filipefurtad0 commented Apr 5, 2023

mkllnk commented Apr 11, 2023

mkllnk commented Apr 11, 2023

filipefurtad0 commented Apr 11, 2023

mkllnk commented Apr 11, 2023

	This report is taking longer to process.
	Sorry, this report took too long to process.

		expect_any_instance_of(Admin::ReportsController).to receive(:sleep).
		and_raise(Rack::Timeout::RequestTimeoutException.new(nil))

[Hidden] Display a friendly message when a background report times out #10644

[Hidden] Display a friendly message when a background report times out #10644

Conversation

mkllnk commented Apr 3, 2023 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

jibees left a comment

Choose a reason for hiding this comment

mkllnk commented Apr 3, 2023

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Apr 5, 2023

mkllnk commented Apr 11, 2023

mkllnk commented Apr 11, 2023

filipefurtad0 commented Apr 11, 2023

mkllnk commented Apr 11, 2023

mkllnk commented Apr 3, 2023 •

edited

Loading