Added API metrics to SQS common worker#6248
Conversation
mzurita-amz
requested review from
KarstenSchnitter,
chenqi0805,
dinujoh,
dlvenable,
engechas,
graytaylor0,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
mzurita-amz
force-pushed
the
sqs-error-metrics
branch
from
9a75be1 to
cf724f7
Compare
|
|
||
| public void recordSqsException(final AwsServiceException e) { | ||
| if ((e instanceof SqsException && ((SqsException) e).statusCode() == 403) || | ||
| e instanceof KmsAccessDeniedException) { |
There was a problem hiding this comment.
Currently OSIS does not perform any client-side encryption of KMS keys. Only SQS performs this. Because this Java exception comes from the AWS SDK in response to an SQS API request, there should be no KMS exceptions directly. All exceptions should be SqsException.
This code can be simplified with that as well since e instanceof SqsException can be used before all of these status code checks.
There was a problem hiding this comment.
I see that these KmsXYZ exceptions do come from SQS. I should have looked at the imports.
import software.amazon.awssdk.services.sqs.model.KmsAccessDeniedException;
So this does get thrown, right?
There was a problem hiding this comment.
yeah, if the notification queue has been defined with KMS encryption, the ReceiveMessages call can throw these exceptions if the caller role has not enough permissions to interact with the KMS key.
There was a problem hiding this comment.
but let me fix some of these casts... I mistakenly added them from a previous revision and are not needed.
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| prometheusMeterRegistry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT); |
There was a problem hiding this comment.
Does this require having a Prometheus server running?
If so, please add a README.md to the sqs-source project describing the setup needed.
There was a problem hiding this comment.
There is no need for Prometheus server. This registry only exposes a scrape method that returns Prometheus metrics in plain-text.
There was a problem hiding this comment.
We don't need an IT for this. We can fix later though.
mzurita-amz
force-pushed
the
sqs-error-metrics
branch
from
cf724f7 to
0bef954
Compare
Signed-off-by: Manuel Mangas Zurita <mzurita@amazon.com>
mzurita-amz
force-pushed
the
sqs-error-metrics
branch
from
0bef954 to
992dd73
Compare
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| prometheusMeterRegistry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT); |
There was a problem hiding this comment.
We don't need an IT for this. We can fix later though.
3 participants
Description
tl;dr; AWS SQS emits some of their exceptions without the matching HTTP code. As we want to generate an aggregate version of these exceptions, we have to explicitly catch the type alongside the status code for the ones that leverage the status code (i.e. InvalidAddressException).
Issues Resolved
N/A
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.