Add Capabilities to /info Route
#7977
Conversation
…y correctly add things to that bucket
|
Note to self - update docs for |
kkeirstead
added
the
update-release-notes
|
Let's be sure to also add tests for this |
|
|
||
| namespace Microsoft.Diagnostics.Monitoring.Options | ||
| { | ||
| public class MonitorCapability : IMonitorCapability |
There was a problem hiding this comment.
I'm not sure if this class adds value here. We already have IMonitorCapability and we can have simple concrete implementations for each capability. Each concrete implementation would then be able to have isolated logic to determine if the feature is on or not (some abstraction may be wanted for in-proc features which would have overlap).
There was a problem hiding this comment.
@jander-msft can weigh in, but if I'm understanding this correctly that's more aligned with the initial approach I took, and then we transitioned to this model that made everything standardized
There was a problem hiding this comment.
Maybe a compromise here would be that the class structure stays the same but the default implementing class doesn't have upfront knowledge about all of its capabilities. This could be done at the registration level.
There was a problem hiding this comment.
We can also make the interface public but the class internal
src/Microsoft.Diagnostics.Monitoring.Options/MonitorCapability.cs
Outdated
Show resolved
Hide resolved
| _capability = capability; | ||
| } | ||
|
|
||
| public abstract void PostConfigure(string? name, T options); |
There was a problem hiding this comment.
I think this is ok, but just to offer some additional options for consideration:
- Override seal PostConfigure and have a abstract method similiar to `abstract bool GetIsEnabled(T options). The base class PostConfigure implementation can call GetIsEnabled and PostConfigure(bool) in the implementation.
- Alternatively, Make CapabilityPostConfigureOptions non-abstract and have it take an additonal parameter of Func<T, bool>. During registration this might look something like
RegisterCapability<CallStackOptions>( (CallStackOptions options) => options.Enabled
src/Microsoft.Diagnostics.Monitoring.Options/MonitorCapability.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Diagnostics.Monitoring.Options/MonitorCapabilityConstants.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.Diagnostics.Monitoring.Tool.FunctionalTests/InfoTests.cs
Outdated
Show resolved
Hide resolved
| where TOptions : class | ||
| where TPostOptions : class, IPostConfigureOptions<TOptions> | ||
| { | ||
| services.AddSingleton(new MonitorCapability() { Name = capabilityName }); |
There was a problem hiding this comment.
I'm going to propose an alternative design that (1) allows the capability properties to be read-only, (2) doesn't require relying on the options construction infrastructure, and (3) avoids the possibility of forgetting to register a MonitorCapability and/or a IPostConfigureOptions<T> pairing:
- Add a
IMonitorCapabilityinterface withstring Nameandbool Enabledgetter-only properties. - Instead of
IPostConfigureOptions<T>implementations, just implementIMonitorCapabilityfor each capability. This implementation can importIOptions<T>and then just set theEnabledproperty in its constructor. - Add each
IMonitorCapabilityimplementation as a singleton. - Still use
MonitorCapabilityfor API model purposes; this can be a projection of theIMonitorCapabilityimplementations.
There was a problem hiding this comment.
I was a little unsure about the last point on here, but I think the latest changes should more-so be following what you had in mind.
| [JsonPropertyName("name")] | ||
| public string Name { get; set; } = string.Empty; | ||
| public string Name { get; init; } |
There was a problem hiding this comment.
I would add the [Required] attribute back to make the OpenAPI description such that the property is not nullable.
| [JsonPropertyName("name")] | ||
| public string Name { get; set; } = string.Empty; | ||
| public string Name { get; init; } |
There was a problem hiding this comment.
Can the init be removed from both of the properties since they are set through the constructor? I think STJ should still include them according to https://learn.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializeroptions.ignorereadonlyproperties?view=net-9.0
Summary
Adds a list of capabilities to the
/inforoute - these are always present, and have anenabledproperty for consumers to determine if a feature is present. Note that this does not include information about how a feature is configured (i.e. it may be enabled, but not configured correctly for a given use case). If we'd like to adjust which properties are included just let me know.Release Notes Entry
Add
Capabilitiesto/inforoute.