-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add getQueueUrls and SQS queue name #11151
Conversation
Will this also need a backport? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do you know why sometimes they don't have a timestamp?
How are the scaled_float changes related to the PR description? Perhaps easier to review if multiple PR's?
x-pack/metricbeat/module/aws/s3_daily_storage/s3_daily_storage.go
Outdated
Show resolved
Hide resolved
If we have 7.x branch opened, then this will need a backport to 7.x. Because the changes are not related to ec2 metricset, so no backport to 7.0. |
if err != nil { | ||
m.logger.Error(err.Error()) | ||
event.Error = err | ||
for _, queueURL := range queueURLs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I would advice to not have for
loops inside for
loops especially if they contain quite a bit of code and instead use functions/methods. This gives also the opportunity to give some nice names to the functions and potentially have docs there. This will also help with testability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I don't know in this case if helped or not :-) I moved the for loop into createSQSEvents
. Thanks!
This will need a rebase. |
@@ -12,7 +12,7 @@ | |||
type: long | |||
description: > | |||
TThe number of messages in the queue that are delayed and not available for reading immediately. | |||
- name: messages.not_visible | |||
- name: message.not_visible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this means we don't check today if the fields are documented? I hope we get this coverage when we have the new "data tests".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Will track this in #11226
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think this does not need a changelog as we didn't release it yet?
Yeah no changelog :-) |
While trying to create Kibana Dashboards for SQS, I realized there are two bugs in the code:
sqs metricset is reporting metrics without queue name, which is needed for dashboards.
When there are more than 1 queue in the same region, events get overwritten and only the last queue get reported. This bug is fixed in this PR by querying ListQueues first to get a list of queues and then loop through each queue name to construct and report events.