Allowed derived telemetry providers #3848
Conversation
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why not provide an overload with options?
There was a problem hiding this comment.
This scenario it might make sense to add TOptions : GraphQLTelemetryOptions since if someone wanted a fully custom telemetry provider they should use ConfigureExecution<TProvider>
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) => { });There was a problem hiding this comment.
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
There was a problem hiding this comment.
Not having these extra overloads should not be confusing for any user.
There was a problem hiding this comment.
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.
|
I still need to add tests for the builder overloads but i think this is everything |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3848 +/- ##
==========================================
+ Coverage 84.56% 84.61% +0.05%
==========================================
Files 418 418
Lines 19271 19289 +18
Branches 3024 3028 +4
==========================================
+ Hits 16296 16321 +25
+ Misses 2266 2255 -11
- Partials 709 713 +4 ☔ View full report in Codecov by Sentry. |
No description provided.