Added AWS SDK metrics to s3 and sqs clients via micrometer#6237
Added AWS SDK metrics to s3 and sqs clients via micrometer#6237mzurita-amz wants to merge 1 commit into
Conversation
viquer
left a comment
viquer
left a comment
There was a problem hiding this comment.
Do we need to add this on any public documentation? Like https://docs.aws.amazon.com/opensearch-service/latest/developerguide/monitoring-pipeline-metrics.html ?
dlvenable
left a comment
dlvenable
left a comment
There was a problem hiding this comment.
Thank you for the contribution, @mzurita-amz !
| } else if (ex.statusCode() == HttpStatusCode.FORBIDDEN) { | ||
| s3ObjectPluginMetrics.getS3ObjectsFailedAccessDeniedCounter().increment(); | ||
| } else if (ex.statusCode() == HttpStatusCode.THROTTLING) { | ||
| s3ObjectPluginMetrics.getS3ObjectsThrottledCounter().increment(); |
There was a problem hiding this comment.
Do we really need the throttling metric? This can come from the AWS SDK if we wire the metrics up for Micrometer.
| static final String SQS_MESSAGES_RECEIVED_METRIC_NAME = "sqsMessagesReceived"; | ||
| static final String SQS_RECEIVE_MESSAGES_FAILED_METRIC_NAME = "sqsReceiveMessageFailed"; | ||
| static final String SQS_RECEIVE_MESSAGES_ACCESS_DENIED_METRIC_NAME = "sqsReceiveMessagesAccessDenied"; | ||
| static final String SQS_RECEIVE_MESSAGES_THROTTLED_METRIC_NAME = "sqsReceiveMessagesThrottled"; |
There was a problem hiding this comment.
Similar comment as with S3. Do we really need throttling?
| @Test | ||
| void processSqsMessages_should_increment_throttled_counter_when_SqsException_with_throttling() { | ||
| SqsException exception = (SqsException) SqsException.builder() | ||
| .message("Throttled") |
There was a problem hiding this comment.
Two comments on these Exceptions.
- You should be able to mock these. We can avoid the builder and any changes on the AWS SDK in this regard.
- Including the
messagehints that we may be reading it. But, the main code is not (which I'm glad of). There is no need to set the message. Certainly not with mocking. If you must use the SDK, just give a random UUID.
mzurita-amz
force-pushed
the
add-s3-sqs-throttling-metrics
branch
2 times, most recently
from
c357351 to
0879edb
Compare
mzurita-amz
changed the title
|
Addressed comments by just using micrometer with SDK integration. |
| Thread.sleep(2000); | ||
|
|
||
| assertTrue(meterRegistry.getMeters().stream() | ||
| .anyMatch(meter -> meter.getId().getName().startsWith("aws.s3.")), | ||
| "Expected S3 metrics to be generated"); | ||
|
|
There was a problem hiding this comment.
Does it make sense to use awaitability to wait for the solution instead of a fixed time?
There was a problem hiding this comment.
Yes, we should avoid sleeps in our tests. They already take a long time.
Could you use mocks instead?
| */ | ||
| public static MetricPublisher createMetricPublisher(final String servicePrefix) { | ||
| final MeterRegistry meterRegistry = Metrics.globalRegistry; | ||
| return new MetricPublisher() { |
There was a problem hiding this comment.
Can we use instead the same model that we have here, what does this path give us as extra rather than use MicrometerMetricPublisher ?
mzurita-amz
force-pushed
the
add-s3-sqs-throttling-metrics
branch
from
0879edb to
c4e0104
Compare
| Thread.sleep(2000); | ||
|
|
||
| assertTrue(meterRegistry.getMeters().stream() | ||
| .anyMatch(meter -> meter.getId().getName().startsWith("aws.s3.")), | ||
| "Expected S3 metrics to be generated"); | ||
|
|
There was a problem hiding this comment.
Yes, we should avoid sleeps in our tests. They already take a long time.
Could you use mocks instead?
mzurita-amz
force-pushed
the
add-s3-sqs-throttling-metrics
branch
from
c4e0104 to
7c2ba56
Compare
- Added common AWSSdkMetrics class so it can be reused in different modules - Added integration tests to ensure remote calls generate metrics prefixed with the service name Signed-off-by: Manuel Mangas Zurita <mzurita@amazon.com>
mzurita-amz
force-pushed
the
add-s3-sqs-throttling-metrics
branch
2 times, most recently
from
2b985a4 to
9377b2f
Compare
|
Discarding this PR in favor of explicitly adding metrics to the catch blocks. |
3 participants
Description
Issues Resolved
N/A
### Testing
Sample metric emitted by EMF Meter Registry
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.