chore: address PR feedback

- Use static mapping instead of multiple if/else-ifs. - Remove the endpoint_id metric tracking since it was removed from the spec. - Make method's name to be verb based. - Remove the getMetricsBuilder from the CommandInterface.
aws · Nov 14, 2024 · f6ed0f9 · f6ed0f9
1 parent 64854e1
commit f6ed0f9
Show file tree

Hide file tree

Showing 9 changed files with 60 additions and 150 deletions.
diff --git a/src/AwsClient.php b/src/AwsClient.php
@@ -438,7 +438,6 @@ private function addSignatureMiddleware(array $args)
         $name = $this->config['signing_name'];
         $region = $this->config['signing_region'];
         $signingRegionSet = $this->signingRegionSet;
-        $handlerList = $this->getHandlerList();
 
         if (isset($args['signature_version'])
          || isset($this->config['configured_signature_version'])
@@ -457,8 +456,7 @@ private function addSignatureMiddleware(array $args)
                 $region,
                 $signatureVersion,
                 $configuredSignatureVersion,
-                $signingRegionSet,
-            $handlerList
+                $signingRegionSet
         ) {
             if (!$configuredSignatureVersion) {
                 if (!empty($command['@context']['signing_region'])) {

diff --git a/src/Command.php b/src/Command.php
@@ -121,7 +121,8 @@ public function get($name)
     }
 
     /**
-     * @inheridoc
+     * Returns the metrics builder instance tied up to this command.
+     *
      * @internal
      *
      * @return MetricsBuilder

diff --git a/src/CommandInterface.php b/src/CommandInterface.php
@@ -39,11 +39,4 @@ public function hasParam($name);
      * @return HandlerList
      */
     public function getHandlerList();
-
-    /**
-     * Returns the metrics builder instance tied up to this command.
-     *
-     * @return MetricsBuilder
-     */
-    public function getMetricsBuilder();
 }
diff --git a/src/Credentials/CredentialSources.php b/src/Credentials/CredentialSources.php
@@ -5,7 +5,7 @@
 /**
  * @internal
  */
-class CredentialSources
+final class CredentialSources
 {
     const STATIC = 'static';
     const ENVIRONMENT = 'env';

diff --git a/src/EndpointV2/EndpointV2Middleware.php b/src/EndpointV2/EndpointV2Middleware.php
@@ -103,10 +103,6 @@ public function __invoke(CommandInterface $command)
             $command->getMetricsBuilder()->append(MetricsBuilder::RESOLVED_ACCOUNT_ID);
         }
         $endpoint = $this->endpointProvider->resolveEndpoint($providerArgs);
-        $command->getMetricsBuilder()->identifyMetricByValueAndAppend(
-            'account_id_endpoint',
-            $endpoint->getUrl()
-        );
         if (!empty($authSchemes = $endpoint->getProperty('authSchemes'))) {
             $this->applyAuthScheme(
                 $authSchemes,

diff --git a/src/MetricsBuilder.php b/src/MetricsBuilder.php
@@ -132,7 +132,6 @@ public function identifyMetricByValueAndAppend(
         static $appendMetricFns = [
             'signature' => 'appendSignatureMetric',
             'request_compression' => 'appendRequestCompressionMetric',
-            'account_id_endpoint' => 'appendAccountIdEndpoint',
             'request_checksum' => 'appendRequestChecksumMetric',
             'credentials' => 'appendCredentialsMetric'
         ];
@@ -171,22 +170,6 @@ private function appendRequestCompressionMetric(string $format): void
         }
     }
 
-    /**
-     * Appends the account id endpoint metric by validating if the
-     * endpoint contains an account id in its URL.
-     *
-     * @param string $endpoint
-     *
-     * @return void
-     */
-    private function appendAccountIdEndpoint(string $endpoint): void
-    {
-        $regex = "/^(https?:\/\/\d{12}\.[^\s\/$.?#].\S*)$/";
-        if (preg_match($regex, $endpoint)) {
-            $this->append(MetricsBuilder::ACCOUNT_ID_ENDPOINT);
-        }
-    }
-
     /**
      * Appends the request checksum metric based on the algorithm.
      *
@@ -227,30 +210,34 @@ private function appendCredentialsMetric(
             return;
         }
 
-        if ($source === CredentialSources::STATIC) {
-            $this->append(MetricsBuilder::CREDENTIALS_CODE);
-        } elseif ($source === CredentialSources::ENVIRONMENT) {
-            $this->append(MetricsBuilder::CREDENTIALS_ENV_VARS);
-        } elseif ($source === CredentialSources::ENVIRONMENT_STS_WEB_ID_TOKEN) {
-            $this->append(MetricsBuilder::CREDENTIALS_ENV_VARS_STS_WEB_ID_TOKEN);
-        } elseif ($source === CredentialSources::STS_ASSUME_ROLE) {
-            $this->append(MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE);
-        } elseif ($source === CredentialSources::STS_WEB_ID_TOKEN) {
-            $this->append(MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE_WEB_ID);
-        } elseif ($source === CredentialSources::PROFILE) {
-            $this->append(MetricsBuilder::CREDENTIALS_PROFILE);
-        } elseif ($source === CredentialSources::IMDS) {
-            $this->append(MetricsBuilder::CREDENTIALS_IMDS);
-        } elseif ($source === CredentialSources::ECS) {
-            $this->append(MetricsBuilder::CREDENTIALS_HTTP);
-        } elseif ($source === CredentialSources::PROFILE_STS_WEB_ID_TOKEN) {
-            $this->append(MetricsBuilder::CREDENTIALS_PROFILE_STS_WEB_ID_TOKEN);
-        } elseif ($source === CredentialSources::PROCESS) {
-            $this->append(MetricsBuilder::CREDENTIALS_PROCESS);
-        } elseif ($source === CredentialSources::SSO) {
-            $this->append(MetricsBuilder::CREDENTIALS_SSO);
-        } elseif ($source === CredentialSources::SSO_LEGACY) {
-            $this->append(MetricsBuilder::CREDENTIALS_SSO_LEGACY);
+        static $credentialsMetricMapping = [
+            CredentialSources::STATIC =>
+                MetricsBuilder::CREDENTIALS_CODE,
+            CredentialSources::ENVIRONMENT =>
+                MetricsBuilder::CREDENTIALS_ENV_VARS,
+            CredentialSources::ENVIRONMENT_STS_WEB_ID_TOKEN =>
+                MetricsBuilder::CREDENTIALS_ENV_VARS_STS_WEB_ID_TOKEN,
+            CredentialSources::STS_ASSUME_ROLE =>
+                MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE,
+            CredentialSources::STS_WEB_ID_TOKEN =>
+                MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE_WEB_ID,
+            CredentialSources::PROFILE =>
+                MetricsBuilder::CREDENTIALS_PROFILE,
+            CredentialSources::IMDS =>
+                MetricsBuilder::CREDENTIALS_IMDS,
+            CredentialSources::ECS =>
+                MetricsBuilder::CREDENTIALS_HTTP,
+            CredentialSources::PROFILE_STS_WEB_ID_TOKEN =>
+                MetricsBuilder::CREDENTIALS_PROFILE_STS_WEB_ID_TOKEN,
+            CredentialSources::PROCESS =>
+                MetricsBuilder::CREDENTIALS_PROCESS,
+            CredentialSources::SSO =>
+                MetricsBuilder::CREDENTIALS_SSO,
+            CredentialSources::SSO_LEGACY =>
+                MetricsBuilder::CREDENTIALS_SSO_LEGACY,
+        ];
+        if (isset($credentialsMetricMapping[$source])) {
+            $this->append($credentialsMetricMapping[$source]);
         }
     }
 
@@ -269,25 +256,18 @@ private function appendCredentialsMetric(
      */
     private function canMetricBeAppended(string $newMetric): bool
     {
+        if ($newMetric === "") {
+            return false;
+        }
+
         if ($this->metricsSize
             + (strlen($newMetric) + strlen(self::$METRIC_SEPARATOR))
             > self::$MAX_METRICS_SIZE
         ) {
-            @trigger_error(
-                "The metric `{$newMetric}` "
-                . "can not be added due to size constraints",
-                E_USER_WARNING
-            );
-
             return false;
         }
 
         if (isset($this->metrics[$newMetric])) {
-            @trigger_error(
-                'The metric ' . $newMetric. ' is already appended!',
-                E_USER_WARNING
-            );
-
             return false;
         }
 

diff --git a/src/UserAgentMiddleware.php b/src/UserAgentMiddleware.php
@@ -16,20 +16,20 @@ class UserAgentMiddleware
 {
     const AGENT_VERSION = 2.1;
     static $userAgentFnList = [
-        'sdkVersion',
-        'userAgentVersion',
-        'hhvmVersion',
-        'osName',
-        'langVersion',
-        'execEnv',
-        'endpointDiscovery',
-        'appId',
-        'metrics'
+        'getSdkVersion',
+        'getUserAgentVersion',
+        'getHhvmVersion',
+        'getOsName',
+        'getLangVersion',
+        'getExecEnv',
+        'getEndpointDiscovery',
+        'getAppId',
+        'getMetrics'
     ];
     static $metricsFnList = [
-        'endpointMetric',
-        'accountIdModeMetric',
-        'retryConfigMetric',
+        'appendEndpointMetric',
+        'appendAccountIdModeMetric',
+        'appendRetryConfigMetric',
     ];
 
     /** @var callable  */
@@ -138,7 +138,7 @@ private function buildUserAgentValue(): array
      *
      * @return string
      */
-    private function sdkVersion(): string
+    private function getSdkVersion(): string
     {
         return 'aws-sdk-php/' . Sdk::VERSION;
     }
@@ -148,7 +148,7 @@ private function sdkVersion(): string
      *
      * @return string
      */
-    private function userAgentVersion(): string
+    private function getUserAgentVersion(): string
     {
         return 'ua/' . self::AGENT_VERSION;
     }
@@ -159,7 +159,7 @@ private function userAgentVersion(): string
      *
      * @return string
      */
-    private function hhvmVersion(): string
+    private function getHhvmVersion(): string
     {
         if (defined('HHVM_VERSION')) {
             return 'HHVM/' . HHVM_VERSION;
@@ -173,7 +173,7 @@ private function hhvmVersion(): string
      *
      * @return string
      */
-    private function osName(): string
+    private function getOsName(): string
     {
         $disabledFunctions = explode(',', ini_get('disable_functions'));
         if (function_exists('php_uname')
@@ -193,7 +193,7 @@ private function osName(): string
      *
      * @return string
      */
-    private function langVersion(): string
+    private function getLangVersion(): string
     {
         return 'lang/php#' . phpversion();
     }
@@ -203,7 +203,7 @@ private function langVersion(): string
      *
      * @return string
      */
-    private function execEnv(): string
+    private function getExecEnv(): string
     {
         if ($executionEnvironment = getenv('AWS_EXECUTION_ENV')) {
             return $executionEnvironment;
@@ -218,7 +218,7 @@ private function execEnv(): string
      *
      * @return string
      */
-    private function endpointDiscovery(): string
+    private function getEndpointDiscovery(): string
     {
         $args = $this->args;
         if (isset($args['endpoint_discovery'])) {
@@ -243,7 +243,7 @@ private function endpointDiscovery(): string
      *
      * @return string
      */
-    private function appId(): string
+    private function getAppId(): string
     {
         if (empty($this->args['app_id'])) {
             return "";
@@ -257,7 +257,7 @@ private function appId(): string
      *
      * @return string
      */
-    private function metrics(): string
+    private function getMetrics(): string
     {
         foreach (self::$metricsFnList as $fn) {
             $this->{$fn}();
@@ -275,7 +275,7 @@ private function metrics(): string
      * Appends the endpoint metric into the metrics builder,
      * just if a custom endpoint was provided at client construction.
      */
-    private function endpointMetric(): void
+    private function appendEndpointMetric(): void
     {
         if (!empty($this->args['endpoint'])) {
             $this->metricsBuilder->append(MetricsBuilder::ENDPOINT_OVERRIDE);
@@ -286,7 +286,7 @@ private function endpointMetric(): void
      * Appends the account id endpoint mode metric into the metrics builder,
      * based on the account id endpoint mode provide as client argument.
      */
-    private function accountIdModeMetric(): void
+    private function appendAccountIdModeMetric(): void
     {
         $accountIdMode = $this->args['account_id_endpoint_mode'] ?? null;
         if ($accountIdMode === null) {
@@ -306,7 +306,7 @@ private function accountIdModeMetric(): void
      * Appends the retry mode metric into the metrics builder,
      * based on the resolved retry config mode.
      */
-    private function retryConfigMetric(): void
+    private function appendRetryConfigMetric(): void
     {
         $retries = $this->args['retries'] ?? null;
         if ($retries === null) {

diff --git a/tests/MetricsBuilderTest.php b/tests/MetricsBuilderTest.php
@@ -61,29 +61,6 @@ static function ( $errno, $errstr ) {
         }
     }
 
-    public function testEmitMetricsSizeConstraintWarning()
-    {
-        try {
-            // Prevent deprecation warning for expectWarning
-            set_error_handler(
-                static function ( $errno, $errstr ) {
-                    throw new \Exception( $errstr, $errno );
-                },
-                E_ALL
-            );
-            $this->expectException(Exception::class);
-            $this->expectExceptionMessage(
-                "The metric `A` can not be added due to size constraints"
-            );
-            $metricsBuilder = new MetricsBuilder();
-            $firstMetric = str_repeat("*", 1024);
-            $metricsBuilder->append($firstMetric);
-            $metricsBuilder->append("A");
-        } finally {
-            restore_error_handler();
-        }
-    }
-
     public function testGetMetricsBuilderFromCommand()
     {
         $command = new Command('TestCommand', [], new HandlerList());

diff --git a/tests/UserAgentMiddlewareTest.php b/tests/UserAgentMiddlewareTest.php
@@ -650,41 +650,6 @@ public function testUserAgentCaptureGzipRequestCompressionMetric()
         ]);
     }
 
-    /**
-     * Tests user agent captures the account id endpoint metric.
-     *
-     * @return void
-     */
-    public function testUserAgentCaptureAccountIdEndpointMetric()
-    {
-        $dynamoDbClient = new DynamoDbClient([
-            'region' => 'us-east-2',
-            'credentials' => new Credentials(
-                'foo',
-                'foo',
-                'foo',
-                null,
-                '123456789012'
-            ),
-            'http_handler' => function (
-                RequestInterface $request
-            ) {
-                $metrics = $this->getMetricsAsArray($request);
-
-                $this->assertTrue(
-                    in_array(MetricsBuilder::ACCOUNT_ID_ENDPOINT, $metrics)
-                );
-
-                return new Response(
-                    200,
-                    [],
-                    '{}'
-                );
-            }
-        ]);
-        $dynamoDbClient->listTables();
-    }
-
     /**
      * Tests user agent captures a resolved account id metric.
      *