Enhance GraphQL OTEL instrumentation with custom metrics and traces #2673
Conversation
RubenCerna2079
requested review from
Aniruddh25,
aaronburtle,
sezal98,
ravishetye,
neeraj-sharma2592 and
sourabh1007
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the OTEL instrumentation for GraphQL APIs by incorporating custom traces and metrics. Key changes include adding new telemetry using directives and namespaces, integrating activity tracking in query execution, and updating the BuildRequestStateMiddleware to support custom telemetry for GraphQL operations.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Added telemetry using directive. |
| src/Service/Controllers/RestController.cs | Updated telemetry namespace usage to align with telemetry enhancements. |
| src/Core/Telemetry/TelemetryTracesHelper.cs | Updated namespace and provided clarified telemetry trace comments. |
| src/Core/Telemetry/TelemetryMetricsHelper.cs | Updated namespace and removed unneeded using statements. |
| src/Core/Services/ExecutionHelper.cs | Added telemetry activity tracking for both query and mutation execution. |
| src/Core/Services/BuildRequestStateMiddleware.cs | Integrated telemetry, runtime config provider, and error tracking. |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
RubenCerna2079
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
| statusCode = HttpStatusCode.InternalServerError; | ||
| } | ||
|
|
||
| string errorMessage = context.Result.Errors![0].Message; |
There was a problem hiding this comment.
We should verify Errors is not null here before dereferencing it
| Exception ex = new(errorMessage); | ||
|
|
||
| // Activity will track error | ||
| activity?.TrackRestControllerActivityFinishedWithException(ex, statusCode); |
There was a problem hiding this comment.
nit: the name TrackRestControllerActivityFinishedWithException is misleading since this is now being used for GraphQL too.. Consider using a generic TrackController.. name
| @@ -91,6 +96,8 @@ public async ValueTask ExecuteQueryAsync(IMiddlewareContext context) | |||
| /// </param> | |||
| public async ValueTask ExecuteMutateAsync(IMiddlewareContext context) | |||
| { | |||
| using Activity? activity = StartQueryActivity(context); | |||
There was a problem hiding this comment.
When is the end of this activity marked?
| // We want to ignore introspection queries DAB uses to check access to GraphQL since they are not sent by the user. | ||
| if (!isIntrospectionQuery) | ||
| { | ||
| TelemetryMetricsHelper.IncrementActiveRequests(apiType); |
There was a problem hiding this comment.
How did you test this? Could you please add it to the description?
Can we add some integration tests as well?
|
You should follow open telemetry semantic conventions for otel metrics and trace. There are few HTTP metrics already available, https://opentelemetry.io/docs/specs/semconv/http/http-metrics/. Similar exercise, we did for Cosmos DB SDK. ref. https://opentelemetry.io/docs/specs/semconv/database/cosmosdb/. Right now, it is under preview. @Aniruddh25 what do you think about it? |
Why make this change?
This closes #2642
What is this change?
This PR enhances the OTEL instrumentation for the GraphQL APIs by adding custom traces and metrics.
Metrics can be filtered for
status_code,api_type,endpointandmethod.How was this tested?
Sample Request(s)