Metrics: Add interfaces and initial implementations #1362
Conversation
| /** | ||
| * Service ID of the AWS service that the API request is made against | ||
| */ | ||
| Service("Service", DEFAULT), |
There was a problem hiding this comment.
This enum needs to be updated to use upper case
| public Builder metricConfigurationProvider(MetricConfigurationProvider metricConfigurationProvider) { | ||
| this.metricConfigurationProvider = metricConfigurationProvider; | ||
| return this; | ||
| } |
| /** | ||
| * A NoOp implementation of {@link MetricRegistry} interface. | ||
| */ | ||
| @SdkProtectedApi |
| * Utilities used for handling shared metric tasks during the request lifecycle. | ||
| */ | ||
| @SdkInternalApi | ||
| public final class MetricUtil { |
There was a problem hiding this comment.
How is this util class different from MetricUtils? Can we put them into one class?
There was a problem hiding this comment.
Didn't realize MetricUtils existed when I wrote MetricUtil and forgot to go back and combine once I did realize
|
|
||
| @Override | ||
| public Set<MetricCategory> metricCategories() { | ||
| return categories; |
There was a problem hiding this comment.
should return immutable set
There was a problem hiding this comment.
In this case, can switch the resolveCategories to use an immutable set. In the future, it's possible to change these at runtime so this would introduce a bunch of copying as this method is called frequently...
| public static final ExecutionAttribute<MetricPublisherConfiguration> METRIC_PUBLISHER_CONFIGURATION = | ||
| new ExecutionAttribute<>("MetricPublisherConfiguration"); | ||
|
|
||
| protected MetricExecutionAttribute() { |
| @@ -102,9 +114,16 @@ private RetryExecutor(SdkHttpFullRequest request, RequestExecutionContext contex | |||
| } | |||
|
|
|||
| public CompletableFuture<Response<OutputT>> execute(CompletableFuture<Response<OutputT>> future) throws Exception { | |||
| MetricRegistry attemptRegistry = MetricUtil.newRegistry(context.executionAttributes()); | |||
There was a problem hiding this comment.
Would it make sense to move line 117 - line 120 to beforeExecute method
There was a problem hiding this comment.
Would need to store a reference to the timer then.
| @@ -119,7 +122,11 @@ public MakeAsyncHttpRequestStage(TransformingAsyncResponseHandler<OutputT> respo | |||
| .fullDuplex(isFullDuplex(context.executionAttributes())) | |||
| .build(); | |||
|
|
|||
| Timer timer = MetricUtils.timer(context.attemptMetricRegistry(), SdkDefaultMetric.HttpRequestRoundTripLatency); | |||
There was a problem hiding this comment.
Can we extract the timer code to a separate method, say startTimer()?
There was a problem hiding this comment.
What's the benefit here? I can change MetricUtils.timer method to start and return the timer but not really sure how much that is saving...
There was a problem hiding this comment.
To me, the benefit is to isolate metrics logic and to keep executeHttpRequest method smaller, simpler and just doing one thing (though it's already doing multiple things right now)
|
|
||
| MetricRegistry attemptRegistry = MetricUtil.newRegistry(context.executionAttributes()); | ||
| Timer apiCallAttemptTimer = MetricUtils.timer(attemptRegistry, SdkDefaultMetric.ApiCallAttemptLatency); | ||
| apiCallAttemptTimer.start(); |
There was a problem hiding this comment.
Can we extract the timer code to a separate method, say startTimer()?
|
|
||
| client = CodePipelineClient.builder() | ||
| .credentialsProvider(CREDENTIALS_PROVIDER_CHAIN) | ||
| // .overrideConfiguration(configuration) |
There was a problem hiding this comment.
Need to look this class over again. Probably not worthwhile to include until publisher is added
spfink
force-pushed
the
finks-varunkn/metrics
branch
from
739682f to
ed5e529
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Continuing the work from: #1314