HotChocolate GraphQL integration#3004
Merged
Merged
Conversation
dromanol
changed the title
dromanol
changed the title
dromanol
added
the
area:automatic-instrumentation
This comment has been minimized.
This comment has been minimized.
pierotibou
reviewed
Jul 22, 2022
pierotibou
left a comment
pierotibou
left a comment
Contributor
There was a problem hiding this comment.
Thanks a lot for doing that. Here's a first set of review as I haven't finished.
My main comments/question are:
- should we merge more things with the graphql integration as both should behave the same functionally (i mean in Datadog)
- don't hesitate to be ;ore verbose in your PR description. The more you say, the less questions we ask ;)
This comment has been minimized.
This comment has been minimized.
dromanol
force-pushed
the
dani/hotchocolate_clean
branch
from
July 24, 2022 11:28
a600040 to
4732650
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
andrewlock
reviewed
Jul 26, 2022
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Nice work, thanks! 👍 I have a bunch of small nits, mostly around some optimisations, but overall this looks really good to me.
The main things to look at is adding testing for multiple package versions (currently it's only testing the latest version)
dromanol
force-pushed
the
dani/hotchocolate_clean
branch
from
August 4, 2022 08:54
32e95ba to
9b22989
Compare
This comment has been minimized.
This comment has been minimized.
dromanol
force-pushed
the
dani/hotchocolate_clean
branch
from
August 7, 2022 15:59
77d6f70 to
ccb139e
Compare
This comment has been minimized.
This comment has been minimized.
pierotibou
approved these changes
Aug 11, 2022
pierotibou
left a comment
pierotibou
left a comment
Contributor
There was a problem hiding this comment.
Thanks for doing this. I've left a few comments and +1 a few Andrew's one.
If you were to fix things and get a second approval, please wait for #2982 to be merged before merging this one please 🙏. That will avoid an extra change of snapshots for Lucas. Though it will mean that you'll need to rebase and update the snapshots once he has merged
This comment has been minimized.
This comment has been minimized.
dromanol
force-pushed
the
dani/hotchocolate_clean
branch
from
August 17, 2022 19:07
897b41e to
797e671
Compare
This comment has been minimized.
This comment has been minimized.
dromanol
force-pushed
the
dani/hotchocolate_clean
branch
from
September 1, 2022 08:49
797e671 to
4818415
Compare
Member
Code Coverage Report 📊✔️ Merging #3004 into master will not change line coverage
View the full report for further details: Datadog.Trace Breakdown ✔️
The following classes have significant coverage changes.
The following classes were added in #3004:
7 classes were removed from Datadog.Trace in #3004 View the full reports for further details: |
dromanol
force-pushed
the
dani/hotchocolate_clean
branch
2 times, most recently
from
September 1, 2022 20:36
ccdc927 to
9f092c2
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Andrew Lock <[email protected]>
Co-authored-by: Pierre Bonet <[email protected]>
…QL/HotChocolate/HotChocolateCommon.cs Co-authored-by: Pierre Bonet <[email protected]>
dromanol
force-pushed
the
dani/hotchocolate_clean
branch
from
September 2, 2022 13:08
9f092c2 to
7cfd665
Compare
Member
Benchmarks Report 🐌Benchmarks for #3004 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ More allocations
|
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑netcoreapp3.1 | 1.34 KB | 1.34 KB | 8 B | 0.60% |
| Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑net472 | 1.41 KB | 1.42 KB | 8 B | 0.57% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | ExecuteAsync |
net472 | 2.54μs | 4.45ns | 16.7ns | 0.224 | 0 | 0 | 1.41 KB |
| master | ExecuteAsync |
netcoreapp3.1 | 1.72μs | 3.89ns | 14.5ns | 0.0179 | 0 | 0 | 1.34 KB |
| #3004 | ExecuteAsync |
net472 | 2.6μs | 5.8ns | 22.5ns | 0.225 | 0 | 0 | 1.42 KB |
| #3004 | ExecuteAsync |
netcoreapp3.1 | 1.71μs | 3.95ns | 15.3ns | 0.0185 | 0 | 0 | 1.34 KB |
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendAsync |
net472 | 5.76μs | 14.2ns | 54.9ns | 0.438 | 0 | 0 | 2.77 KB |
| master | SendAsync |
netcoreapp3.1 | 3.62μs | 8.85ns | 34.3ns | 0.0343 | 0 | 0 | 2.6 KB |
| #3004 | SendAsync |
net472 | 5.71μs | 12.8ns | 49.6ns | 0.439 | 0 | 0 | 2.77 KB |
| #3004 | SendAsync |
netcoreapp3.1 | 3.53μs | 8.22ns | 31.8ns | 0.0341 | 0 | 0 | 2.6 KB |
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 3.06μs | 2.98ns | 11.5ns | 0.287 | 0 | 0 | 1.81 KB |
| master | EnrichedLog |
netcoreapp3.1 | 2.47μs | 1.59ns | 5.96ns | 0.0246 | 0 | 0 | 1.85 KB |
| #3004 | EnrichedLog |
net472 | 3.24μs | 4.13ns | 16ns | 0.287 | 0 | 0 | 1.81 KB |
| #3004 | EnrichedLog |
netcoreapp3.1 | 2.58μs | 1.18ns | 4.27ns | 0.0245 | 0 | 0 | 1.85 KB |
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 152μs | 88.3ns | 342ns | 0.683 | 0.228 | 0 | 4.65 KB |
| master | EnrichedLog |
netcoreapp3.1 | 116μs | 138ns | 533ns | 0 | 0 | 0 | 4.49 KB |
| #3004 | EnrichedLog |
net472 | 151μs | 125ns | 468ns | 0.686 | 0.229 | 0 | 4.65 KB |
| #3004 | EnrichedLog |
netcoreapp3.1 | 116μs | 261ns | 1.01μs | 0.0587 | 0 | 0 | 4.49 KB |
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 5.66μs | 9.49ns | 36.7ns | 0.568 | 0.00284 | 0 | 3.59 KB |
| master | EnrichedLog |
netcoreapp3.1 | 4.39μs | 7.56ns | 28.3ns | 0.0547 | 0 | 0 | 3.91 KB |
| #3004 | EnrichedLog |
net472 | 5.67μs | 25ns | 97ns | 0.568 | 0.00283 | 0 | 3.59 KB |
| #3004 | EnrichedLog |
netcoreapp3.1 | 4.41μs | 9.21ns | 35.7ns | 0.0535 | 0 | 0 | 3.91 KB |
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendReceive |
net472 | 2.22μs | 1.12ns | 4.18ns | 0.218 | 0 | 0 | 1.37 KB |
| master | SendReceive |
netcoreapp3.1 | 1.88μs | 7.85ns | 30.4ns | 0.0176 | 0 | 0 | 1.32 KB |
| #3004 | SendReceive |
net472 | 2.31μs | 2.45ns | 9.48ns | 0.218 | 0 | 0 | 1.37 KB |
| #3004 | SendReceive |
netcoreapp3.1 | 1.8μs | 1.06ns | 3.82ns | 0.0179 | 0 | 0 | 1.32 KB |
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 5.06μs | 0.834ns | 3.12ns | 0.352 | 0 | 0 | 2.23 KB |
| master | EnrichedLog |
netcoreapp3.1 | 4.22μs | 0.964ns | 3.73ns | 0.0233 | 0 | 0 | 1.8 KB |
| #3004 | EnrichedLog |
net472 | 4.97μs | 2.37ns | 9.17ns | 0.352 | 0 | 0 | 2.23 KB |
| #3004 | EnrichedLog |
netcoreapp3.1 | 4.45μs | 1.52ns | 5.7ns | 0.0243 | 0 | 0 | 1.8 KB |
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net472 | 1.18μs | 0.447ns | 1.67ns | 0.129 | 0 | 0 | 810 B |
| master | StartFinishSpan |
netcoreapp3.1 | 1.01μs | 0.213ns | 0.767ns | 0.0101 | 0 | 0 | 760 B |
| master | StartFinishScope |
net472 | 1.37μs | 0.645ns | 2.41ns | 0.141 | 0 | 0 | 891 B |
| master | StartFinishScope |
netcoreapp3.1 | 1.09μs | 0.316ns | 1.14ns | 0.0116 | 0 | 0 | 880 B |
| #3004 | StartFinishSpan |
net472 | 1.19μs | 0.361ns | 1.4ns | 0.128 | 0 | 0 | 810 B |
| #3004 | StartFinishSpan |
netcoreapp3.1 | 918ns | 0.443ns | 1.71ns | 0.0102 | 0 | 0 | 760 B |
| #3004 | StartFinishScope |
net472 | 1.42μs | 0.581ns | 2.17ns | 0.141 | 0 | 0 | 891 B |
| #3004 | StartFinishScope |
netcoreapp3.1 | 1.14μs | 10.7ns | 101ns | 0.0117 | 0 | 0 | 880 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net472 | 1.51μs | 0.623ns | 2.41ns | 0.141 | 0 | 0 | 891 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 1.25μs | 0.423ns | 1.58ns | 0.0119 | 0 | 0 | 880 B |
| #3004 | RunOnMethodBegin |
net472 | 1.48μs | 0.502ns | 1.88ns | 0.142 | 0 | 0 | 891 B |
| #3004 | RunOnMethodBegin |
netcoreapp3.1 | 1.19μs | 0.835ns | 3.23ns | 0.0119 | 0 | 0 | 880 B |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary of changes
HotChocolate v11-v12 Tracer integration and sample application (chose the most popular latest versions)
Reason for change
Implement integration with Hotchocolate GraphQL .net library
Implementation details
Instrumented HotChocolate.Execution.RequestExecutor.ExecuteAsync
This captures the main boundary of the operation and all semantic errors.
Instrumented HotChocolate.Execution.Processing.QueryExecutor from
This captures the operation type if it is well formed. For V11 it only captures Query types.
Instrumented HotChocolate.Execution.Processing.MutationExecutorfrom for V11
This captures the Mutation operation type if it is well formed.
Moved .Net GraphQL specific implementatiosn to Datadog.Trace.ClrProfiler.AutoInstrumentation.GraphQL.Net namespace
HotChocolate specific implementation set on namespace Datadog.Trace.ClrProfiler.AutoInstrumentation.GraphQL.HotChocolate
Common code left on Datadog.Trace.ClrProfiler.AutoInstrumentation.GraphQL
Test coverage
Other details