Tweak telemetry events on batch middleware #1170
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewhr
added a commit
to andrewhr/opentelemetry-erlang-contrib
that referenced
this pull request
May 27, 2022
Add instrumentation for `Absinthe`, the GraphQL Elixir-framwork. Relies on
some new telemetry work on Absinthe:
* [Added telemetry for async helper][1], used to propagate traces
* [Improved telemetry for batch helper][2], used to propagate traces
* [Tweaked pipeline instrumentation][3], that splits parse/validate/execute
phases, following the approach of [JavaScript libraries][4]
Span names and attributes also follow the [JavaScript impl][4], the
closes to a future [Semantic Attribute][5] spec.
The "span tracker" used here is to connect traces in a shape that
resembles the query shape instead of execution shape. One example is:
```
query {
users {
name
}
}
```
In case `name` is an async resolver (for example), the execution will
resembles a recursive function, and such our trace will be:
```
RootQueryType.users
User.name
User.name
User.name
```
With the span tracker, the above trace will become:
```
RootQueryType.users
User.name
User.name
User.name
```
That helps visually scan the traces, and it's friendly to folks who
doesn't know much about Absinthe internals.
[1]: absinthe-graphql/absinthe#1169
[2]: absinthe-graphql/absinthe#1170
[3]: absinthe-graphql/absinthe#1172
[4]: https://www.npmjs.com/package/@opentelemetry/instrumentation-graphql
[5]: open-telemetry/opentelemetry-specification#2456
With a similar rationale from [this PR][1], tweak the telemetry events from batch middleware. The existing event contract stays the same, but move to run from inside the Task used for batch execution. There is a small semantic difference of when the :stop event fires - this patch sends the event at the end of the task, while the current form may have a delay/sync caused by Task await. A complementary event is added as well to announce when the Resolver continuation - the post batch - happens. Given that piece is part of the resolver, the resolution struct is included as part of the metadata. [1]: absinthe-graphql#1169
andrewhr
force-pushed
the
batch-middleware-telemetry-tweaks
branch
from
f873819
to
719e54c
Compare
Add docs for both Async and Batch events, covering the recent changes made in relation to both sets. The docs are written following conventions found in many libraries that adopted `:telemetry` since the first implemention done at Absinthe. The original text wasn't touched, but there is room for improvement there as well. Given the amount of events, some grouping was done by "middleware".
andrewhr
force-pushed
the
batch-middleware-telemetry-tweaks
branch
from
719e54c
to
8c36f75
Compare
andrewhr
commented
Jun 15, 2022
| :telemetry.span([:absinthe, :middleware, :async, :task], %{}, fn -> {fun.(), %{}} end) | ||
| :telemetry.span([:absinthe, :middleware, :async, :task], %{}, fn -> | ||
| result = fun.() | ||
| {result, %{result: result}} |
There was a problem hiding this comment.
Sorry for this non-related change, but as I've wrote the telemetry docs I felt like could be a good consistency change as batch telemetry events returns results as well.
|
Add docs for both Async and Batch events, covering the recent changes made in relation to both sets. The docs are written following conventions found in many libraries that adopted Given the amount of events, I've grouped them by middleware, and phrased sections so Any thoughts @benwilson512? Thanks in advance! |
andrewhr
commented
Jun 15, 2022
| "erlex": {:hex, :erlex, "0.2.6", "c7987d15e899c7a2f34f5420d2a2ea0d659682c06ac607572df55a43753aa12e", [:mix], [], "hexpm", "2ed2e25711feb44d52b17d2780eabf998452f6efda104877a3881c2f8c0c0c75"}, | ||
| "ex_doc": {:hex, :ex_doc, "0.27.3", "d09ed7ab590b71123959d9017f6715b54a448d76b43cf909eb0b2e5a78a977b2", [:mix], [{:earmark_parser, "~> 1.4.19", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "ee60b329d08195039bfeb25231a208749be4f2274eae42ce38f9be0538a2f2e6"}, | ||
| "ex_doc": {:hex, :ex_doc, "0.28.4", "001a0ea6beac2f810f1abc3dbf4b123e9593eaa5f00dd13ded024eae7c523298", [:mix], [{:earmark_parser, "~> 1.4.19", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "bf85d003dd34911d89c8ddb8bda1a958af3471a274a4c2150a9c01c78ac3f8ed"}, |
There was a problem hiding this comment.
Took the opportunity to update the library. If there are other preferred means to do so, I'm glad to revert.
The `:monotonic_time` is part of [1.1.0 release of :telemetry][1]. Enforce that version at minimum to match documented Telemetry for Async and Batch middleware events. [1]: https://github.com/beam-telemetry/telemetry/blob/091121f6153840fd079e68940715a5c35c5aa445/CHANGELOG.md#110
andrewhr
force-pushed
the
batch-middleware-telemetry-tweaks
branch
from
8de92a3
to
05c7318
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With a similar rationale from this PR, tweak the telemetry events from batch middleware.
The existing event contract stays the same, but move to run from inside the Task used for batch execution. There is a small semantic difference of when the :stop event fires - this patch sends the event at the end of the task, while the current form may have a delay/sync caused by Task await.
A complementary event is added as well to announce when the Resolver continuation - the post batch - happens. Given that piece is part of the resolver, the resolution struct is included as part of the metadata.