Skip to content
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

packages/aws/data_stream/lambda: Add Async metrics #9782

Merged
merged 11 commits into from May 6, 2024

Conversation

agithomas
Copy link
Contributor

@agithomas agithomas commented May 3, 2024

Proposed commit message

  • Add async event age and async event drops metrics.
  • Add sum aggregation metrics for Throttle, Errors, DeadLetterErrors, DestinationDeliveryFailures metrics.
  • Update Error dashboard with sum aggregation metrics
  • Update dashboard with async event age and async event drop metrics

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • elastic-package build
  • elastic-package stack up -v -d --services package-registry
  • Dashboard data verification
  • Package upgrade verification
  • Index mapping verification

How to test this PR locally

  • elastic-package build
  • elastic-package stack up -v -d --services package-registry

Screenshots

image

@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@agithomas agithomas marked this pull request as ready for review May 4, 2024 06:15
@agithomas agithomas requested review from a team as code owners May 4, 2024 06:15
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits only

@aliabbas-elastic aliabbas-elastic self-requested a review May 6, 2024 05:47
Copy link
Contributor

@aliabbas-elastic aliabbas-elastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nits and dashboard changes requested. Dashboard changes are not specific with the changes in this PR but they are kind of minor improvements which can be done alongside

packages/aws/data_stream/lambda/fields/fields.yml Outdated Show resolved Hide resolved
packages/aws/data_stream/lambda/fields/fields.yml Outdated Show resolved Hide resolved
packages/aws/changelog.yml Outdated Show resolved Hide resolved
packages/aws/img/metricbeat-aws-lambda-overview.png Outdated Show resolved Hide resolved
Copy link
Member

@shmsr shmsr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check all avg. Error average fields will have "per minute" in the definition as the aggregates are done on one-minute intervals.

Please check unchanged avg fields if they have the average mentioned in their description.

packages/aws/data_stream/lambda/fields/fields.yml Outdated Show resolved Hide resolved
- name: AsyncEventsReceived.sum
type: long
metric_type: gauge
description: The number of events that Lambda successfully queues for processing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: The number of events that Lambda successfully queues for processing.
description: The number of asynchronous events that Lambda successfully adds to the function event queue.

packages/aws/data_stream/lambda/fields/fields.yml Outdated Show resolved Hide resolved
@shmsr shmsr added the enhancement New feature or request label May 6, 2024
@shmsr shmsr changed the title Add AWS Lambda Async metrics packages/aws/data_stream/lambda: Add Async metrics May 6, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing came to my mind seeing the updated screenshot. For visualizations Async Event Wait and Async Events Received their @timestamp interval is set to Auto which is correct and the timestamp per x mins/secs/hours would change dynamically as per the duration selected above. However for Top Errors and Lambda Function Duration same is not reflected. Can we check there if something else is set in there?

@agithomas
Copy link
Contributor Author

Please check all avg. Error average fields will have "per minute" in the definition as the aggregates are done on one-minute intervals.

This is not true. The blog was merely mentioning on how the in-built cloud watch dashboard is built on.

image

In elastic integration, the user has the flexibility to choose the Period and aggregations occur based on the configured Period value.

@shmsr
Copy link
Member

shmsr commented May 6, 2024

Please check all avg. Error average fields will have "per minute" in the definition as the aggregates are done on one-minute intervals.

This is not true. The blog was merely mentioning on how the in-built cloud watch dashboard is built on.

image

In elastic integration, the user has the flexibility to choose the Period and aggregations occur based on the configured Period value.

Got it. Then let's not add the per minute change I suggested. Or we can replace "per minute" to "per configured period". Is it by default 1m?

@agithomas
Copy link
Contributor Author

Got it. Then let's not add the per minute change I suggested. Or we can replace "per minute" to "per configured period". Is it by default 1m?

The period settings is applicable for all the fields in this integration. Specifically calling out for this field alone may not be needed, i think

@agithomas agithomas requested a review from shmsr May 6, 2024 10:23
Copy link
Contributor

@aliabbas-elastic aliabbas-elastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @agithomas

Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@agithomas agithomas merged commit 3202bcd into elastic:main May 6, 2024
5 checks passed
@elasticmachine
Copy link

Package aws - 2.15.0 containing this change is available at https://epr.elastic.co/search?package=aws

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants