Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allowed derived telemetry providers #3848

Merged
merged 10 commits into from
Dec 24, 2023
Merged

Allowed derived telemetry providers #3848

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dfb98d8
Allowed derived telemetry providers
Shane32 Dec 23, 2023
b50a94a
Fix tests
Shane32 Dec 23, 2023
bb41d16
Update
Shane32 Dec 23, 2023
2c77fc2
Fix private
Shane32 Dec 23, 2023
529d003
Update comments
Shane32 Dec 23, 2023
e3bb197
Merge branch 'master' into telemetry_derived
Shane32 Dec 23, 2023
b16d0a3
Add additional overloads
Shane32 Dec 23, 2023
aaa975d
Add more tests
Shane32 Dec 24, 2023
e657377
Test 7 and 8
Shane32 Dec 24, 2023
814d024
Update url
Shane32 Dec 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/GraphQL.ApiTests/net50/GraphQL.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,10 @@ namespace GraphQL
where TMiddleware : class, GraphQL.Instrumentation.IFieldMiddleware { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry(this GraphQL.DI.IGraphQLBuilder builder, System.Action<GraphQL.Telemetry.GraphQLTelemetryOptions>? configure = null) { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry(this GraphQL.DI.IGraphQLBuilder builder, System.Action<GraphQL.Telemetry.GraphQLTelemetryOptions, System.IServiceProvider>? configure) { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry<TProvider>(this GraphQL.DI.IGraphQLBuilder builder)
where TProvider : GraphQL.Telemetry.GraphQLTelemetryProvider { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry<TProvider>(this GraphQL.DI.IGraphQLBuilder builder, TProvider telemetryProvider)
where TProvider : GraphQL.Telemetry.GraphQLTelemetryProvider { }
public static GraphQL.IConfigureAutoSchema WithMutation<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.None | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicMethods | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties)] TMutationClrType>(this GraphQL.IConfigureAutoSchema builder) { }
public static GraphQL.IConfigureAutoSchema WithSubscription<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.None | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicMethods | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties)] TSubscriptionClrType>(this GraphQL.IConfigureAutoSchema builder) { }
}
Expand Down
4 changes: 4 additions & 0 deletions src/GraphQL.ApiTests/net60/GraphQL.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,10 @@ namespace GraphQL
where TMiddleware : class, GraphQL.Instrumentation.IFieldMiddleware { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry(this GraphQL.DI.IGraphQLBuilder builder, System.Action<GraphQL.Telemetry.GraphQLTelemetryOptions>? configure = null) { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry(this GraphQL.DI.IGraphQLBuilder builder, System.Action<GraphQL.Telemetry.GraphQLTelemetryOptions, System.IServiceProvider>? configure) { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry<TProvider>(this GraphQL.DI.IGraphQLBuilder builder)
where TProvider : GraphQL.Telemetry.GraphQLTelemetryProvider { }
public static GraphQL.DI.IGraphQLBuilder UseTelemetry<TProvider>(this GraphQL.DI.IGraphQLBuilder builder, TProvider telemetryProvider)
where TProvider : GraphQL.Telemetry.GraphQLTelemetryProvider { }
public static GraphQL.IConfigureAutoSchema WithMutation<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.None | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicMethods | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties)] TMutationClrType>(this GraphQL.IConfigureAutoSchema builder) { }
public static GraphQL.IConfigureAutoSchema WithSubscription<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.None | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicMethods | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties)] TSubscriptionClrType>(this GraphQL.IConfigureAutoSchema builder) { }
}
Expand Down
5 changes: 4 additions & 1 deletion src/GraphQL.Tests/Instrumentation/OpenTelemetryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,12 @@ public void CanInitializeTelemetryViaReflection()
// verify that the telemetry service is added implicitly
var services = new ServiceCollection();
services.AddGraphQL(_ => { });
var serviceDescriptor = services.SingleOrDefault(x => x.ImplementationType == typeof(GraphQLTelemetryProvider)).ShouldNotBeNull();
var serviceDescriptor = services.SingleOrDefault(x => x.ImplementationInstance == GraphQLTelemetryProvider.AutoTelemetryProvider).ShouldNotBeNull();
serviceDescriptor.ServiceType.ShouldBe(typeof(IConfigureExecution));
serviceDescriptor.Lifetime.ShouldBe(Microsoft.Extensions.DependencyInjection.ServiceLifetime.Singleton);

// ensure auto-telemetry is still enabled
OpenTelemetry.AutoInstrumentation.Initializer.Enabled.ShouldBeTrue();
}
finally
{
Expand Down
17 changes: 4 additions & 13 deletions src/GraphQL/DI/GraphQLBuilderBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
using GraphQL.Execution;
#if NET5_0_OR_GREATER
using GraphQL.Telemetry;
#endif
using GraphQL.Types;
using GraphQL.Types.Relay;
using GraphQL.Validation;
Expand Down Expand Up @@ -78,19 +81,7 @@ protected virtual void RegisterDefaultServices()
{
// this will run prior to any other calls to UseTelemetry
// it will also cause telemetry to be called first in the pipeline
this.UseTelemetry(OpenTelemetry.AutoInstrumentation.Initializer.Options != null
? (opts =>
{
var autoOpts = OpenTelemetry.AutoInstrumentation.Initializer.Options;
opts.RecordDocument = autoOpts.RecordDocument;
opts.SanitizeDocument = autoOpts.SanitizeDocument;
opts.Filter = autoOpts.Filter;
opts.EnrichWithExecutionOptions = autoOpts.EnrichWithExecutionOptions;
opts.EnrichWithDocument = autoOpts.EnrichWithDocument;
opts.EnrichWithExecutionResult = autoOpts.EnrichWithExecutionResult;
opts.EnrichWithException = autoOpts.EnrichWithException;
})
: null);
this.ConfigureExecution(GraphQLTelemetryProvider.AutoTelemetryProvider);
}
#endif
}
Expand Down
22 changes: 22 additions & 0 deletions src/GraphQL/Extensions/GraphQLBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,7 @@ public static IGraphQLBuilder AddExecutionStrategy<TExecutionStrategy>(this IGra
/// Configures the GraphQL engine to collect traces via the <see cref="System.Diagnostics.Activity">System.Diagnostics.Activity API</see> and records events that match the
/// <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/graphql.md">OpenTelemetry recommendations</see>.
/// Trace data contains the GraphQL operation name, the operation type, and the optionally the document.
/// Disables auto-instrumentation for GraphQL.
/// </summary>
/// <remarks>
/// When applicable, place after calls to UseAutomaticPersistedQueries to ensure that the query document is recorded properly.
Expand All @@ -1206,10 +1207,31 @@ public static IGraphQLBuilder UseTelemetry(this IGraphQLBuilder builder, Action<
/// <inheritdoc cref="UseTelemetry(IGraphQLBuilder, Action{GraphQLTelemetryOptions}?)"/>
public static IGraphQLBuilder UseTelemetry(this IGraphQLBuilder builder, Action<GraphQLTelemetryOptions, IServiceProvider>? configure)
{
OpenTelemetry.AutoInstrumentation.Initializer.Enabled = false;
builder.Services.Configure(configure);
builder.Services.TryRegister<IConfigureExecution, GraphQLTelemetryProvider>(ServiceLifetime.Singleton, RegistrationCompareMode.ServiceTypeAndImplementationType);
return builder;
}

/// <inheritdoc cref="UseTelemetry(IGraphQLBuilder, Action{GraphQLTelemetryOptions}?)"/>
public static IGraphQLBuilder UseTelemetry<TProvider>(this IGraphQLBuilder builder)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you pass the GraphQLTelemetryOptions? And if TProvider : GraphQLTelemetryProvider, it may be useful to make TOptions : GraphQLTelemetryOptions, so the options can be extended to support additional configuration for the extended provider.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as ErrorInfoProvider and other builder methods. The user will have to configure options with Configure() to configure the options class they are using.

Copy link
Contributor

@gao-artur gao-artur Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not provide an overload with options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scenario it might make sense to add TOptions : GraphQLTelemetryOptions since if someone wanted a fully custom telemetry provider they should use ConfigureExecution<TProvider>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whereas IErrorInfoProvider does not require any options; just the default implementation allows for certain options. Users should be able to write their own implementation without necessarily using options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, basically I'd just do all 4 together on request. But if you're requesting them then I'd just add all 4 now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right. I recall now that I used this pattern when need to create options with dependencies

services.AddOptions<IConfiguration>()
    .Configure<GraphQLTelemetryOptions>((configuration, options) => { });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to think of it is that people can just AddProvider<T> and they should be quite familiar with DI and the fact that if you have an options class, they need to AddSingleton to register it

Copy link
Member Author

@Shane32 Shane32 Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having these extra overloads should not be confusing for any user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, basically I'd just do all 4 together on request. But if you're requesting them then I'd just add all 4 now.

Please add them. Because I just copied the UseTelemetry method and updated it to register the derived type, because I didn't find a simpler way to do that.

where TProvider : GraphQLTelemetryProvider
{
OpenTelemetry.AutoInstrumentation.Initializer.Enabled = false;
builder.ConfigureExecution<TProvider>();
return builder;
}

/// <inheritdoc cref="UseTelemetry(IGraphQLBuilder, Action{GraphQLTelemetryOptions}?)"/>
public static IGraphQLBuilder UseTelemetry<TProvider>(this IGraphQLBuilder builder, TProvider telemetryProvider)
where TProvider : GraphQLTelemetryProvider
{
if (telemetryProvider == null)
throw new ArgumentNullException(nameof(telemetryProvider));
OpenTelemetry.AutoInstrumentation.Initializer.Enabled = false;
builder.ConfigureExecution(telemetryProvider);
return builder;
}
#endif
#endregion
}
25 changes: 24 additions & 1 deletion src/GraphQL/Telemetry/GraphQLTelemetryProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public class GraphQLTelemetryProvider : IConfigureExecution
{
private readonly GraphQLTelemetryOptions _telemetryOptions;
private const string ACTIVITY_OPERATION_NAME = "graphql";
private readonly bool _onlyWhenAutoTelemetryEnabled;

/// <summary>
/// Returns an <see cref="System.Diagnostics.ActivitySource"/> instance to be used for GraphQL.NET telemetry.
Expand All @@ -44,11 +45,33 @@ public GraphQLTelemetryProvider(GraphQLTelemetryOptions options)
_telemetryOptions = options;
}

/// <summary>
/// Initializes a new instance that only executes when <see cref="OpenTelemetry.AutoInstrumentation.Initializer.Enabled"/>
/// is <see langword="true"/>, using the options from <see cref="OpenTelemetry.AutoInstrumentation.Initializer.Options"/>.
/// </summary>
private GraphQLTelemetryProvider()
{
_onlyWhenAutoTelemetryEnabled = true;
_telemetryOptions = OpenTelemetry.AutoInstrumentation.Initializer.Options ?? new GraphQLTelemetryOptions();
}

// make sure no accidental use of the default constructor by DI by making it private,
// accessible only through AutoTelemetryProvider, a shared static instance
internal static GraphQLTelemetryProvider? _autoTelemetryProvider;
Shane32 marked this conversation as resolved.
Show resolved Hide resolved
internal static GraphQLTelemetryProvider AutoTelemetryProvider => _autoTelemetryProvider ??= new GraphQLTelemetryProvider();

/// <inheritdoc/>
public virtual float SortOrder => GraphQLBuilderExtensions.SORT_ORDER_CONFIGURATION;

/// <inheritdoc/>
public virtual async Task<ExecutionResult> ExecuteAsync(ExecutionOptions options, ExecutionDelegate next)
public virtual Task<ExecutionResult> ExecuteAsync(ExecutionOptions options, ExecutionDelegate next)
{
return !_onlyWhenAutoTelemetryEnabled || OpenTelemetry.AutoInstrumentation.Initializer.Enabled
? ExecuteInternalAsync(options, next)
: next(options);
Shane32 marked this conversation as resolved.
Show resolved Hide resolved
}

private async Task<ExecutionResult> ExecuteInternalAsync(ExecutionOptions options, ExecutionDelegate next)
{
if (!_telemetryOptions.Filter(options))
return await next(options).ConfigureAwait(false);
Expand Down
Loading