Dataloader integration #21
Conversation
Per: https://github.com/opencensus-beam/opencensus_absinthe/pull/13/files#r290670529 This resolves a race condition at compile-time. See upstream PR here: opencensus-beam#13
Codecov Report
@@ Coverage Diff @@
## master #21 +/- ##
==========================================
- Coverage 22.22% 14.81% -7.41%
==========================================
Files 6 8 +2
Lines 36 54 +18
==========================================
Hits 8 8
- Misses 28 46 +18
Continue to review full report at Codecov.
|
… batch unique span names
…-batch unique span names
That'll do. I'm not hurting in production from not having any attributes on the blueprint span. Knowing which resolver is slow is most of the battle. I won't be able to review any of these changes this week, I'm afraid. |
|
I swear, it'll end up there'll be some way to process the middleware chain to wrap the arguments to the async and batch middleware before they invoke |
README.md
Outdated
| @@ -82,6 +82,14 @@ end | |||
|
|
|||
| [c:middleware/3]: https://hexdocs.pm/absinthe/Absinthe.Schema.html#c:middleware/3 | |||
|
|
|||
| If you're using [`Dataloader`][dataloader], you will want to use the provided | |||
There was a problem hiding this comment.
Maybe I'm just too much of an [RFC 2119 fan, but can we get the word SHOULD in here?
lib/opencensus/absinthe.ex
Outdated
| @@ -41,6 +41,14 @@ defmodule Opencensus.Absinthe do | |||
|
|
|||
| Worst case, you'll need to copy the code from the current `pipeline` target and add a call to | |||
| `Opencensus.Absinthe.add_phases/1` as above. | |||
|
|
|||
| If you're using [`Dataloader`][dataloader], you will want to use the provided | |||
There was a problem hiding this comment.
If this is in a @moduledoc or @doc, you can refer directly to the Dataloader and Opencensus.Absinthe.Middleware.Dataloader documentation by the module name. You don't need to construct your own links.
| @@ -12,15 +12,20 @@ defmodule Opencensus.Absinthe.Middleware do | |||
| @impl true | |||
| @spec call(Resolution.t(), term()) :: Resolution.t() | |||
| def call(%Resolution{state: :unresolved} = resolution, field: field) do | |||
| acc = Acc.get(resolution) | |||
| case Acc.get(resolution) do | |||
| # nil -> | |||
There was a problem hiding this comment.
I'd prefer to guard against this with some unit tests rather than commented out warnings. :)
There was a problem hiding this comment.
I've added better comments and removed the commented-out warning 👍
| end | ||
| end) | ||
|
|
||
| span_ctx = :oc_trace.start_span("resolution_#{counter || 0}", acc.span_ctx, span_options) |
There was a problem hiding this comment.
I love the counter! Can we make that part of the main accumulator so we can track how many times we looped the loop regardless of the mechanism? I'm OK with an extra span per overall resolution named "resolution 0" etc.
Let's also put the counter to the resolution span attributes as "absinthe.blueprint.resolution", and the blueprint span attributes as "absinthe.blueprint.resolution_count". I'm pretty sure:ocp.put_span_attribute/2 and :oc_trace.put_span_attribute/3 won't mind it being put more than once.
There was a problem hiding this comment.
Ok - I think I got this working - if not, please be detailed on what I'm doing wrong. Again, I'm not too familiar with Absinthe. ='(
Thank you very much!
| exec | ||
| |> Acc.set(new_acc) | ||
| |> DefaultDataloader.before_resolution() | ||
| end |
There was a problem hiding this comment.
I recently automated insertion of Opencensus.Absinthe.Middleware on literally every field with a resolver in our code base, simulating what'll happen with Absinthe 1.5 by default.
Turns out, it's a mess when tracing every field meets Absinthe.Resolution.Helpers.dataloader/3. For every item in the output, we get a span starting just before the call to Dataloader.load and ending just after the call to the last on_load in the chain. We need to know how long Dataloader.run/1 took and what it was doing, but the trace can't tell us.
Strikes me if you finish your span right here, we'll know!
Read also, #26, which is all about this.
|
|
||
| acc | ||
| |> Map.get(@span_key) | ||
| |> :oc_trace.finish_span() |
There was a problem hiding this comment.
If we wait until after_resolution/1 to finish the span, it'll include a lot of time spent waiting for other middleware and plugins.
Your |
|
Thanks for the feedback! I'll go over it and try and get something back to you shortly. 👍 |
Resolves #18
Note that at this point in time, mostly due to my unfamiliarity as of yet with the code and the data structures at play, this mostly provides very bland spans. If somebody could inform me how to extract the current field for a given blueprint execution, that would get me 90% there I think