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
Support OpenTelemetry auto instrumentation #3631
Conversation
@@ -1193,7 +1193,7 @@ public static IGraphQLBuilder UseTelemetry(this IGraphQLBuilder builder, Action< | |||
public static IGraphQLBuilder UseTelemetry(this IGraphQLBuilder builder, Action<GraphQLTelemetryOptions, IServiceProvider>? configure) | |||
{ | |||
builder.Services.Configure(configure); | |||
builder.ConfigureExecution<GraphQLTelemetryProvider>(); | |||
builder.Services.TryRegister<IConfigureExecution, GraphQLTelemetryProvider>(ServiceLifetime.Singleton, RegistrationCompareMode.ServiceTypeAndImplementationType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary if #3628 is merged
Before merging, this PR should link to the applicable documentation at https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation which indicates this is the correct pattern to support automatic instrumentation. So far I am unable to find any. |
Maybe this can be used for initial reference. Due you are the most responsive currently, this will be the first implementation. We probably would like to reconfirm it with 1-2 other library authors and after that proper documentation will be released. The first implementations would be probably linked as an actual references. |
@RassK Using the sample code provided in the issue you linked, I created a test, which does not pass because (a) the binding flag needs to be public, not non-public, and (b) the arguments were not passed to the method. Can you provide a link to the reflection code that calls this |
@sungam3r The changes here are minor and do not cause public API changes. The only behavioral change would be that if auto-telemetry were enabled, AND someone called I support merging this change. |
Note: at the moment we are relying @RassK 's test build to know that the code works as intended with the OpenTelemetry SDK. Perhaps someone could add a sample project with a reference to OpenTelemetry and tests to verify it "end to end". But I do not intend to figure out if/how this is possible. |
Updated the issue in OpenTelemetry AutoInstrumentation about the correct usage. The final implementation is here > We do have end to end tests, but at least rc package has to be available in the Nuget to test it. |
I updated the tests in this PR to align as closely as possible with RassK/opentelemetry-dotnet-instrumentation#954. |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #3631 +/- ##
==========================================
- Coverage 83.91% 83.88% -0.03%
==========================================
Files 381 382 +1
Lines 16892 16914 +22
Branches 2718 2720 +2
==========================================
+ Hits 14175 14189 +14
- Misses 2070 2079 +9
+ Partials 647 646 -1
|
No description provided.