-
Notifications
You must be signed in to change notification settings - Fork 653
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
internal: true up internal metrics collection for post-SRA middleware #2642
Conversation
bfb75ee
to
0fd81c8
Compare
ResolveEndpointStartTime time.Time | ||
ResolveEndpointEndTime time.Time | ||
EndpointResolutionDuration time.Duration | ||
GetIdentityStart, GetIdentityEnd time.Time |
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.
Should this be GetIdentityStartTime
to follow the other MetricData members convention?
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 elected to not since I felt it was obvious in the types, but I can change it. Note this API (all of this) is behind a private/"don't use, will break" disclaimer as well.
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'd go with consistency rather than make it obvious with the types
@@ -144,8 +145,6 @@ type AttemptMetrics struct { | |||
ConnRequestedTime time.Time | |||
ConnObtainedTime time.Time | |||
ConcurrencyAcquireDuration time.Duration | |||
CredentialFetchStartTime time.Time |
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 need a bit more context as to why are we removing these
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.
It's no longer a per-attempt metric with the SRA auth refactor - credential (abstractly referred to now as identity) retrieval happened as part of the signing process in the now-deprecated sigv4 middleware, as part of an attempt lifecycle. Now it's its own middleware (GetIdentity
) separate from signing and before the retry middleware i.e. before the attempt loop starts.
Long-term I think we do want to pull the start of the retry loop further back but that's out of scope for now.
gofmt
previously - there are some incidental changes therein