-
Notifications
You must be signed in to change notification settings - Fork 751
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
Current dotnet/extensions API Surface #4373
Conversation
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.
Gone through the NoDocs
API/NoDocs/Microsoft.Extensions.Http.Resilience/EndpointGroup.cs
Outdated
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Resilience.FaultInjection/CustomResultPolicyOptions.cs
Outdated
Show resolved
Hide resolved
API/NoDocs/Microsoft.AspNetCore.Telemetry/HttpLoggingServiceExtensions.cs
Outdated
Show resolved
Hide resolved
API/NoDocs/Microsoft.AspNetCore.Telemetry/HttpLoggingServiceExtensions.cs
Outdated
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.AmbientMetadata/ApplicationMetadataExtensions.cs
Outdated
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.DependencyInjection/AutoActivationExtensions.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Diagnostics.ResourceMonitoring/CountersSetup.cs
Outdated
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Diagnostics.ResourceMonitoring/NullUtilizationExtensions.cs
Outdated
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Diagnostics.ResourceMonitoring/LinuxUtilizationExtensions.cs
Outdated
Show resolved
Hide resolved
API/WithDocs/Microsoft.Extensions.Http.Telemetry.Logging/LoggingOptions.cs
Outdated
Show resolved
Hide resolved
API/WithDocs/Microsoft.Extensions.Compliance.Redaction/RedactionAbstractionsExtensions.cs
Outdated
Show resolved
Hide resolved
...oDocs/Microsoft.Extensions.Diagnostics.HealthChecks/ResourceUtilizationHealthCheckOptions.cs
Show resolved
Hide resolved
@danmoseley Please don't push commits to this PR. This is all auto-generated stuff, so any edits will be lost. |
7d59e47
to
17c9fb7
Compare
@geeknoid is all feedback addressed -- or more to the point, this API dump is now out of date with changes you've made? maybe we should close this now? edit: oh, Jose pointed out that you're keeping this up to date. |
...Microsoft.Extensions.DependencyInjection/HttpResilienceHedgingHttpClientBuilderExtensions.cs
Show resolved
Hide resolved
...NoDocs/Microsoft.Extensions.DependencyInjection/HttpResilienceHttpClientBuilderExtensions.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.DependencyInjection/ResilienceServiceCollectionExtensions.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Http.Resilience/HedgingEndpointOptions.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Http.Resilience/HttpClientHedgingResiliencePredicates.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Http.Resilience/HttpClientHedgingResiliencePredicates.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Http.Resilience/HttpClientResiliencePredicates.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Http.Resilience/HttpClientResiliencePredicates.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.Http.Resilience/HttpClientResiliencePredicates.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.ObjectPool/ObjectPoolServiceCollectionExtensions.cs
Show resolved
Hide resolved
{ | ||
public static IServiceCollection AddFakeRedaction(this IServiceCollection services); | ||
public static IServiceCollection AddFakeRedaction(this IServiceCollection services, Action<FakeRedactorOptions> configure); | ||
} |
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.
Is it missing public static IServiceCollection AddFakeRedaction(this IServiceCollection services, IConfigurationSection section);
API/NoDocs/Microsoft.Extensions.DependencyInjection/HeaderParsingServiceCollectionExtensions.cs
Show resolved
Hide resolved
API/NoDocs/Microsoft.Extensions.DependencyInjection/RedactionServiceCollectionExtensions.cs
Show resolved
Hide resolved
public sealed class CounterAttribute : Attribute | ||
{ | ||
public string? Name { get; set; } | ||
public string[]? TagNames { get; } | ||
public Type? Type { get; } | ||
public CounterAttribute(params string[] tagNames); | ||
public CounterAttribute(Type type); | ||
} |
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.
I'm not a fan of the shape of CounterAttribute
and friends. I could imagine someone entering [Counter("my.cool.counter.name")]
and expecting a counter with that name but instead accidentally calling the CounterAttribute(params string[] tagNames)
ctor and getting a counter with an inferred name and tag named my.cool.counter.name
instead. Feels like pit of failure.
The most significant piece of information for a counter is its name. It should be in the constructor. Also, it should always be required. Infering it from a return type doesn't feel good. Thought should be put into the name.
Also, the Type
property name isn't descriptive. TagsType
is still a little confusing because tags can have a type but I can't think of a better name.
public sealed class CounterAttribute : Attribute | |
{ | |
public string? Name { get; set; } | |
public string[]? TagNames { get; } | |
public Type? Type { get; } | |
public CounterAttribute(params string[] tagNames); | |
public CounterAttribute(Type type); | |
} | |
public sealed class CounterAttribute : Attribute | |
{ | |
public string? Name { get; } | |
public string[]? TagNames { get; set; } | |
public Type? TagsType { get; set; } | |
public CounterAttribute(string name); | |
} |
Usage:
[Counter("my.cool.counter.name", TagNames = [ "one", "two", "etc" ])]
If someone sets TagNames
and TagType
then throw an error.
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.
@geeknoid @dariusclay @iliar-turdushev folks, any thoughts here?
Personally I see the point of the suggested change, but @JamesNK please bear in mind that by default the source-gen will take a type's name as an instrument name, that's why its API was made that way.
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.
I'm good with the suggestion, but it will be a breaking change for existing customers, and complicate their migration to dotnet/extensions. Is it OK?
The only change I'd make to the suggestion is mark name
argument as optional:
public CounterAttribute(string? name = null);
If name
is null
then we infer the name of a counter from the return type of a method.
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.
This comment could have been addressed a few weeks back, but now it's too late. We can no longer do breaking changes in dotnet/extensions. So let's improve this incrementally when possible, without breaking things.
using Microsoft.AspNetCore.Hosting.Server; | ||
using Microsoft.Extensions.Hosting; | ||
|
||
namespace Microsoft.AspNetCore.Testing; |
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.
These extensions should be in the same namespace as their extended types, IWebHostBuilder and IHost (even if it means splitting the class). I'll take care of that.
Please review to make sure you're happy with:
Last chance to speak up before November is upon us.
Microsoft Reviewers: Open in CodeFlow