-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: surface HTTP connection manager metrics #88
Conversation
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
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.
Couple questions/nit but looks good.
/** | ||
* The active metrics for this connection manager | ||
*/ | ||
public val managerMetrics: HttpManagerMetrics |
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.
nit: Wouldn't metrics
suffice here given it's namespaced under HttpClientConnectionManager
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.
Similar to other Java→Kotlin mapping classes, I chose to keep this the same as the underlying property. If you feel strongly I can change it to be simpler (which I would generally prefer when not mapping existing classes).
*/ | ||
package aws.sdk.kotlin.crt.http | ||
|
||
public data class HttpManagerMetrics( |
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.
question: Are these connection manager metrics? If so HttpManager
seems ambiguous and should probably be HttpConnectionManagerMetrics
or HttpConnectionMetrics
fix: Missing docs on these metrics. I know they are meant to match SRA but probably should document them still and any differences/quirks that may lie herein.
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.
Similar to other Java→Kotlin mapping classes, I chose to keep this the same as the underlying class. If you feel strongly I can change it to be clearer (which I would generally prefer when not mapping existing classes).
Issue #, if available:
smithy-lang/smithy-kotlin#893
Description of changes:
This change makes HTTP manager metrics available in the Kotlin API.
Companion PR: smithy-lang/smithy-kotlin#1017
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.