-
Notifications
You must be signed in to change notification settings - Fork 162
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
Move Diagnostics types from Management.Abstractions to Management.Endpoint #1424
base: main
Are you sure you want to change the base?
Conversation
…nt.Abstractions project into Management.Endpoint project
Quality Gate passedIssues Measures |
@@ -9,42 +8,6 @@ Steeltoe.Management.Configuration.EndpointPermissions | |||
Steeltoe.Management.Configuration.EndpointPermissions.Full = 2 -> Steeltoe.Management.Configuration.EndpointPermissions | |||
Steeltoe.Management.Configuration.EndpointPermissions.None = 0 -> Steeltoe.Management.Configuration.EndpointPermissions | |||
Steeltoe.Management.Configuration.EndpointPermissions.Restricted = 1 -> Steeltoe.Management.Configuration.EndpointPermissions | |||
Steeltoe.Management.Diagnostics.DiagnosticObserver |
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.
If we still need DiagnosticObserver
then I think it (and IDiagnosticObserver
) should stay in the Abstractions package for extensibility
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.
Even if we still need them, they remain public and extensible. But it is only used by the metrics actuator, so close to that seems the better place to me.
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.
Keeping it in Abstractions would be better if (as an example) we wanted to add a DiagnosticObserver
for Eureka. Sure it's only used by the endpoint, but another implementation likely wouldn't be in an application and might not want the heavier dependency on Endpoint
Steeltoe.Management.Diagnostics.IDiagnosticsManager | ||
Steeltoe.Management.Diagnostics.IDiagnosticsManager.Start() -> void | ||
Steeltoe.Management.Diagnostics.IDiagnosticsManager.Stop() -> void | ||
Steeltoe.Management.Diagnostics.IRuntimeDiagnosticSource |
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 think there's an argument (though probably undocument) in there somewhere for making IRuntimeDiagnosticSource
extensible, so maybe that should stay here
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.
What would that argument be?
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.
Basically the same as DiagnosticObserver
, though I'm a little less confident on this one. Ultimately, we need to get to the bottom of whether we can remove these entirely
Description
This PR moves all types in the namespace
Steeltoe.Management.Diagnostics
in theSteeltoe.Management.Abstractions
project to theSteeltoe.Management.Endpoint
project.Comparing changes with Steeltoe 3.x, I found that some types moved from Endpoint to Abstractions in v4. Upon further inspection, it turns out all of these types are exclusively used by the Metrics actuator. In Steeltoe 3.x, they were shared between Core/Base projects, which required them to live in Abstractions.
Quality checklist
If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.