-
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
Adding aws-s3 metric for approximate messages waiting #34488
Conversation
…ges-waiting-metric
This pull request is now in conflicts. Could you fix it? 🙏
|
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
x-pack/filebeat/input/awss3/input.go
Outdated
@@ -376,5 +380,16 @@ func getProviderFromDomain(endpoint string, ProviderOverride string) string { | |||
return "unknown" | |||
} | |||
|
|||
func PollSqsWaitingMetric(ctx context.Context, receiver *sqsReader) { |
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.
Does this need to be exported?
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'm not really sure, but I don't think so?
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.
Let's make it unexported.
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 after nits removed.
x-pack/filebeat/input/awss3/input.go
Outdated
@@ -376,5 +380,16 @@ func getProviderFromDomain(endpoint string, ProviderOverride string) string { | |||
return "unknown" | |||
} | |||
|
|||
func PollSqsWaitingMetric(ctx context.Context, receiver *sqsReader) { |
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.
Let's make it unexported.
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.
One issue I have with using the data is for the case of sqs_messages_waiting_gauge=0
, I cannot distinguish between the queue being empty and the client not having permissions to collect the value.
I think the best behavior would be not register the metric's existence until the first time it successfully obtains a value for it. @kgeller @efd6 Or can you think of a better approach?
The only other approach I can think of is to register an obviously bogus count if no access, like -1. But I think that has the potential to introduce unnecessary confusion. |
ISTM that the zero is a zero whether it's from being unavailable or empty. I'd suggest having a second boolean metric indicating that the value is available from the data source. Is there a possibility that it would become unavailable after intially having been available? If so, then the bool should be a guage over some past window of time. |
I pushed what Dan suggested since it helps keep the metric logic straightforward, and it keeps it super clear as to what's going on. |
After discussing with Andrew, we tweaked the plan to be: not initializing the metric until we successfully get a value, then set the metric per the value when polled. If we lose permissions/encounter an issue, we will set the metric to -1. |
My main concern was usability of the data in a Lens visualization. Visualizing a single time-series will be simpler. |
…ges-waiting-metric
… into s3-messages-waiting-metric
What does this PR do?
Adding the metric to answer how many message are waiting (visible) in the SQS queue
Checklist
I have made corresponding change to the default configuration filesCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.How to test this PR locally
http://localhost:5066/dataset?pretty
now includes the followingRelated issues