Added metrics to SQS notification queue for S3 source#6256
Merged
dlvenable merged 1 commit intoNov 13, 2025
Conversation
mzurita-amz
force-pushed
the
fix-sqs-metrics-location
branch
4 times, most recently
from
1165546 to
888b9ce
Compare
mzurita-amz
requested review from
KarstenSchnitter,
chenqi0805,
dinujoh,
dlvenable,
engechas,
graytaylor0,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
viquer
suggested changes
Nov 12, 2025
Comment on lines
+66
to
+68
| public static final String SQS_MESSAGE_ACCESS_DENIED_METRIC_NAME = "sqsMessagesAccessDenied"; | ||
| public static final String SQS_MESSAGE_THROTTLED_METRIC_NAME = "sqsMessagesThrottled"; | ||
| public static final String SQS_QUEUE_NOT_FOUND_METRIC_NAME = "sqsQueueNotFound"; |
dlvenable
requested changes
Nov 12, 2025
Comment on lines
+66
to
+68
| public static final String SQS_MESSAGE_ACCESS_DENIED_METRIC_NAME = "sqsMessagesAccessDenied"; | ||
| public static final String SQS_MESSAGE_THROTTLED_METRIC_NAME = "sqsMessagesThrottled"; | ||
| public static final String SQS_QUEUE_NOT_FOUND_METRIC_NAME = "sqsQueueNotFound"; |
| isStopped = true; | ||
| } | ||
|
|
||
| public void recordSqsException(final AwsServiceException e) { |
| import static org.opensearch.dataprepper.plugins.source.s3.SqsWorker.SQS_VISIBILITY_TIMEOUT_CHANGED_COUNT_METRIC_NAME; | ||
|
|
||
| @ExtendWith(MockitoExtension.class) | ||
| @MockitoSettings(strictness = Strictness.LENIENT) |
Member
There was a problem hiding this comment.
This will apply lenient to all mocks. Let's avoid this. You can use lenient(). on the specific metric mocks.
Relatedly, do you even need lenient in your tests? They look scoped down enough to no require it at all.
dinujoh
reviewed
Nov 12, 2025
| } else if (e.statusCode() == 404 || | ||
| e instanceof QueueDoesNotExistException || | ||
| e instanceof KmsNotFoundException) { | ||
| sqsQueueNotFoundCounter.increment(); |
Member
There was a problem hiding this comment.
e instanceof KmsNotFoundException do we need a new counter sqsKmsKeyNotFoundCounter ?
Contributor
Author
There was a problem hiding this comment.
On our side, aggregating into a single NotFound metric is enough. But let me know if you or David think otherwise. Or a different name for this metric.
Member
There was a problem hiding this comment.
This is a good point.
If we keep one metric, maybe rename it?
static final String SQS_QUEUE_NOT_FOUND_METRIC_NAME = "sqsQueueNotFound";
to
static final String SQS_RESOURCE_NOT_FOUND_METRIC_NAME = "sqsResourceNotFound";
888b9ce to
dd85299
Compare
mzurita-amz
force-pushed
the
fix-sqs-metrics-location
branch
from
dd85299 to
975a52a
Compare
viquer
approved these changes
Nov 12, 2025
dinujoh
previously approved these changes
Nov 12, 2025
mzurita-amz
force-pushed
the
fix-sqs-metrics-location
branch
4 times, most recently
from
d0b5aa3 to
2051700
Compare
dinujoh
reviewed
Nov 12, 2025
| } else if (e.statusCode() == 404 || | ||
| e instanceof QueueDoesNotExistException || | ||
| e instanceof KmsNotFoundException) { | ||
| sqsQueueNotFoundCounter.increment(); |
Member
There was a problem hiding this comment.
We should consider renaming the variable sqsQueueNotFoundCounter to sqsResourceNotFoundCounter
dinujoh
previously approved these changes
Nov 12, 2025
dinujoh
previously approved these changes
Nov 12, 2025
mzurita-amz
force-pushed
the
fix-sqs-metrics-location
branch
from
1402625 to
2f01169
Compare
dlvenable
previously approved these changes
Nov 13, 2025
Member
|
@mzurita-amz , There are failing unit tests for the S3 source. And the DCO check is not passing. Let's address these and then we will be good to merge. |
mzurita-amz
force-pushed
the
fix-sqs-metrics-location
branch
from
2f01169 to
0bef3dd
Compare
mzurita-amz
force-pushed
the
fix-sqs-metrics-location
branch
2 times, most recently
from
b354f95 to
a75d63d
Compare
- Add lenient mocks for SQS counter metrics initialized in SqsWorker constructor - Fix test methods to use processSqsMessages() instead of run() to avoid infinite loops - Ensure all SQS exception tests properly verify counter increments - All 333 tests now pass Signed-off-by: Manuel Mangas Zurita <mzurita@amazon.com>
mzurita-amz
force-pushed
the
fix-sqs-metrics-location
branch
from
a75d63d to
77897fe
Compare
dinujoh
approved these changes
Nov 13, 2025
dlvenable
approved these changes
Nov 13, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
In PR-6248 we added SQS metrics to the Common worker. This actual worker used in S3 collection is the SqsWorker. This PR makes SQS metrics available in S3 source as well.
Also, removed IT from previous received feedback.
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.