enhancement: user agent 2.1 #3001
Conversation
yenfryherrerafeliz
force-pushed
the
feat_user_agent_2.1
branch
from
c1ae03a
to
8d081b1
Compare
yenfryherrerafeliz
force-pushed
the
feat_user_agent_2.1
branch
13 times, most recently
from
f3155b2
to
3cd174e
Compare
This change provides: - A builder class for appending metrics - Default initialization of a metrics builder within command instantiation - Wraps user agent logic into a single middleware class - Adds middlewares into the different features from where metrics can be gather, to acomplish this purpose.
yenfryherrerafeliz
force-pushed
the
feat_user_agent_2.1
branch
from
3cd174e
to
4905534
Compare
| @@ -280,6 +280,11 @@ public function __construct(array $args) | |||
| if (isset($args['with_resolved'])) { | |||
| $args['with_resolved']($config); | |||
| } | |||
| MetricsBuilder::appendMetricsCaptureMiddleware( | |||
There was a problem hiding this comment.
question: do we need to add this (or other static/guaranteed metrics) to our middleware stack?
| @@ -490,6 +497,11 @@ private function addSignatureMiddleware(array $args) | |||
| $region = $signingRegionSet | |||
| ?? $commandSigningRegionSet | |||
| ?? $region; | |||
|
|
|||
| MetricsBuilder::appendMetricsCaptureMiddleware( | |||
There was a problem hiding this comment.
question: This function is invoked during the sign step. How is the signature version being passed to the metrics builder by appending middleware to the build step, which has already completed?
There was a problem hiding this comment.
Good catch. Metrics here will not be collected. I will do a refactor for this type of cases.
| @@ -394,4 +402,23 @@ private function resolveAccountId(): ?string | |||
|
|
|||
| return $identity->getAccountId(); | |||
| } | |||
|
|
|||
| private function hookAccountIdMetric($accountId, &$command) | |||
There was a problem hiding this comment.
question: can we make a generic method on the MetricsBuilder class that will handle this and other things like it i.e. accountidendpointmetric?
There was a problem hiding this comment.
Yes, I can take a look and see if it fits better there.
| } | ||
|
|
||
| /** | ||
| * Appends a metric into the internal metrics holder. |
There was a problem hiding this comment.
nit: can this and other longer docblocks be shortened?
| */ | ||
| private function buildUserAgentValue(): array | ||
| { | ||
| static $fnList = [ |
There was a problem hiding this comment.
suggestion: consider moving to a static property if it could possibly be used by anything else
This change provides:
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.