Skip to content

Add callback_url/acct information for Sidekiq PuSH workers Exception.#4281

Merged
Gargron merged 4 commits intomastodon:masterfrom
clworld:add_destinfo_to_sidekiq_error
Jul 26, 2017
Merged

Add callback_url/acct information for Sidekiq PuSH workers Exception.#4281
Gargron merged 4 commits intomastodon:masterfrom
clworld:add_destinfo_to_sidekiq_error

Conversation

@clworld
Copy link
Copy Markdown
Contributor

@clworld clworld commented Jul 20, 2017 •
edited
Loading

Some exception does not report hostname in exception message.
So, hostname does not shown in Sidekiq error console.
You need check database tables to know which host returns error.
With this PR, you can easily see which server is in trouble...
sidekiq_fix3

beatrix-bitrot
beatrix-bitrot approved these changes Jul 20, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@beatrix-bitrot beatrix-bitrot left a comment

Choose a reason for hiding this comment

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

YES PLEASE

@beatrix-bitrot
Copy link
Copy Markdown
Contributor

beatrix-bitrot commented Jul 20, 2017

ok maybe fix the broken tests d:

still... i really want this

@beatrix-bitrot beatrix-bitrot requested a review from unarist July 20, 2017 14:22
@clworld clworld force-pushed the add_destinfo_to_sidekiq_error branch from 5155e38 to 9f44db1 Compare July 20, 2017 15:56
@clworld clworld force-pushed the add_destinfo_to_sidekiq_error branch from 9f44db1 to a9ff459 Compare July 20, 2017 16:00
nightpool
nightpool reviewed Jul 20, 2017
View reviewed changes
app/lib/exceptions.rb
def to_s
"#{@response.uri} returned code #{@response.code}"
if response.respond_to? :url
super("#{response.uri} returned code #{response.code}")
Copy link
Copy Markdown
Member

@nightpool nightpool Jul 20, 2017

Choose a reason for hiding this comment

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

url vs uri?

Copy link
Copy Markdown
Contributor Author

@clworld clworld Jul 20, 2017

Choose a reason for hiding this comment

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

Oh. mistake…

@nightpool
Copy link
Copy Markdown
Member

nightpool commented Jul 20, 2017

What is the difference between "response.uri" and "acct"? One gives more meaningful information, in my opinion.

@nightpool
Copy link
Copy Markdown
Member

nightpool commented Jul 20, 2017 •
edited
Loading

This feels like it's redundant now that we use UnexpectedResponseError.

@clworld
Copy link
Copy Markdown
Contributor Author

clworld commented Jul 20, 2017

I'm confused about RSpec executes Sidekiq worker directly and exceptions thrown by workers returns to caller...
I choose way that don't change Exception class. but this way assumes all StandardError are reproducible..
Another way is wrap errors by some Exception (Mastodon::WorkerError). It breaks rspec which checks which exception is raised.

@clworld
Copy link
Copy Markdown
Contributor Author

clworld commented Jul 20, 2017

@nightpool
responce.uri is what UnexpectedResponseError show on .message.
I don't change UnexpectedResponseError's output. I changed UnexpectedResponseError to make it reproducible.

What I changed in this PR is add acct/callback_url to Exceptions which Sidekiq workers raises.
callback_url is url to post OStatus payload.
acct is Mastodon account name to subscribe. For example: "[email protected]" is my account's acct.

@nightpool
Copy link
Copy Markdown
Member

nightpool commented Jul 20, 2017

okay, that makes sense. LGTM

unarist
unarist reviewed Jul 20, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@unarist unarist 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, but I'm not sure about raise e.class "..." is proper way or not.

@clworld clworld mentioned this pull request Jul 20, 2017
Closed
@clworld
Copy link
Copy Markdown
Contributor Author

clworld commented Jul 20, 2017

I send alternative implementation: #4283
which don't use 'raise e.class "..."' thing. but wrap all exceptions.

@Gargron Gargron merged commit 994d948 into mastodon:master Jul 26, 2017
hcmiya pushed a commit to hcmiya/v6don that referenced this pull request Jul 29, 2017
@clworld @hcmiya
Add callback_url/acct information for Sidekiq PuSH workers Exception. (…
8a0a785 
…mastodon#4281)

* Add destination informations to exception on SubscribeWorker and DeliveryWorker.

* Simplify delivery error message.

* Prevent changing Exception type...

* fix typo.
@unarist unarist mentioned this pull request Sep 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nightpool nightpool nightpool left review comments

@Gargron Gargron Gargron approved these changes

+2 more reviewers

@unarist unarist unarist left review comments

@beatrix-bitrot beatrix-bitrot beatrix-bitrot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@clworld @beatrix-bitrot @nightpool @Gargron @unarist