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

Add log if ES is unreachable #1315

Closed
wants to merge 3 commits into from
Closed

Add log if ES is unreachable #1315

wants to merge 3 commits into from

Conversation

pedrorijo91
Copy link
Member

If ES is down, we were swallowing the exception. Now we at least log it as an error. Not sure if we have warnings at heroku level (@peff ), but if we don't, it won't hurt

closes #1001

@peff peff temporarily deployed to git-scm-pr-1315 February 24, 2019 19:32 Inactive
@pedrorijo91 pedrorijo91 temporarily deployed to git-scm-pr-1315 February 24, 2019 19:42 Inactive
@peff
Copy link
Member

peff commented Feb 25, 2019

Wouldn't the exception eventually make it all the way to the top-level and cause Rails to dump a backtrace to the log (and a 500 to the user)?

But if we're not logging it currently, I'm not at all opposed to doing so. We don't keep the logs themselves for very long, and there's far too much volume to probably want to keep them all. But every once in a while capture a snapshot with heroku logs -t if I'm actively debugging something.

@pedrorijo91
Copy link
Member Author

Wouldn't the exception eventually make it all the way to the top-level and cause Rails to dump a backtrace to the log (and a 500 to the user)?

but do we want to provide 500 for the user?

@peff
Copy link
Member

peff commented Feb 25, 2019

but do we want to provide 500 for the user?

What should we provide? The only other obvious answer is an empty search result, but I wonder if a 500 makes more sense from the point of caching (i.e., it really is a server error, and if they try again in a minute they're going to get a different answer). I doubt it matters much in practice, though.

@pedrorijo91
Copy link
Member Author

sure, I can make the exception bubble up. Will do that this weekend probably

@pedrorijo91
Copy link
Member Author

it seems some tests broke. will need to take care of that

@pedrorijo91
Copy link
Member Author

finally got some time to look into the failing tests, and I'm not sure of the best approach:

  1. change the test/test_helper.rb file to stub any possible missing call - I'm not even 100% sure what's this file doing
  2. mock the Doc.search and Section.search methods (which will call the elastic API):
  test "should get search page" do
    allow(Doc).to receive(:search).with(any_args).and_return([])
    allow(Section).to receive(:search).with(any_args).and_return([])

    get :search, search: "git-rebase"
    assert_response :success
  end

but this will return Minitest::UnexpectedError: NoMethodError: undefined method allow' for #SiteControllerTest:0x00007f8288fcd930`, which makes me think I'm missing some import.

But if I add require "rails_helper" on top of the test/functional/site_controller_test.rb file, I get 'require': cannot load such file -- rails_helper (LoadError)

I'm pretty sure I'm doing some rails-noob mistake here. Any hints @peff / @tarebyte ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails does not complain if no ElasticSearch instance is running
2 participants