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

Raise informative error on batched resolver timeout #1180

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jordi-chacon
Copy link

When the Task that runs a batched resolver times out we currently raise a generic error like this:

** (exit) exited in: Task.await(%Task{owner: #PID<0.15353.21>, pid: #PID<0.15380.21>, ref: #Reference<0.2441152191.2237136897.192020>}, 5000)

This provides no context to the engineer debugging the error about the resolver that timed out, making debugging much harder.

This change raises a RuntimeError which includes the resolver that timed out along with the arguments it received:

Batch resolver timed out after 1 ms.
Batch fun: {Absinthe.Middleware.BatchTest.Schema, :slow_field_by_user_id}
Batch data: [3, 2, 1]
Batch opts: [timeout: 1]

This should make the debugging experience a lot better.

@benwilson512
Copy link
Contributor

This is a great change, can you add a changelog entry?

Task.await(task, timeout)
catch
:exit, {:timeout, {Task, :await, _}} ->
raise """
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main thing I need to sort out is whether turning this exit into a raise is going to be an issue.

Task.await/2 emits an exit when it times out, instead of a RuntimeError that raise emits.

@maartenvanvliet can you think of any reason why this could be a problem?

jordi-chacon and others added 2 commits September 3, 2023 09:45
When the Task that runs a batched resolver times out we currently
raise a generic error like this:

** (exit) exited in: Task.await(%Task{owner: #PID<0.15353.21>, pid: #PID<0.15380.21>, ref: #Reference<0.2441152191.2237136897.192020>}, 5000)

This provides no context to the engineer debugging the error about the resolver
that timed out, making debugging much harder.

This change raises a RuntimeError which includes the resolver that timed out
along with the arguments it received:

Batch resolver timed out after 1 ms.
Batch fun: {Absinthe.Middleware.BatchTest.Schema, :slow_field_by_user_id}
Batch data: [3, 2, 1]
Batch opts: [timeout: 1]

This should make the debugging experience a lot better.
@jordi-chacon jordi-chacon force-pushed the raise-informative-error-on-batched-resolver-timeout branch from 7a889c6 to 22ea9c2 Compare September 3, 2023 07:46
@rossvz
Copy link

rossvz commented May 14, 2024

Bumping an old PR to see what it would take to get this merged. We see hundreds of these errors and we use batch resolvers everywhere so its hard to track down!

If there's hesitations around raise vs emitting exit - could the code be tweaked to Logger.error or something similar?

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.

3 participants