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.

Sign up for GitHub

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

Jump to bottom

Fix a couple of bugs in the logic for how AWS metric periods are calculated #32724

Merged
merged 6 commits into from
Aug 25, 2022
Merged

Fix a couple of bugs in the logic for how AWS metric periods are calculated #32724

merged 6 commits into from
Aug 25, 2022

Conversation

tommyers-elastic
Copy link
Contributor

@tommyers-elastic tommyers-elastic commented Aug 17, 2022 •
edited
Loading

Firstly, we clarify that periods are always whole-minute durations.
Any period that is less than or in between minutes is rounded up to the
next whole-minute.

Secondly, we ensure that the resulting time period is always in the past.
This stops us getting empty metrics for the current-minute period.

Thirdly, we follow the AWS guidelines of aligning periods to whole
multiples within the hour e.g. 10:25->10:30 instead of 10:27->10:32
for a 5 minute period.

Fixes #32687
Fixes #32640

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 17, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @tommyers-elastic? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)
    To fixup this pull request, you need to add the backport labels for the needed
    branches, such as:
  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@tommyers-elastic
Fix a couple of bugs in the logic for how AWS metric periods are calc…
19e7073 
…uated.

Firstly, we clarify that periods are always whole-minute durations.
Any period that is less than or in between minutes is rounded up to the
next whole-minute.

Secondly, we ensure that the resulting time period is always in the past.
This stops us getting empty metrics for the current-minute period.

Thirdly, we follow the AWS guidelines of aligning periods to whole
multiples within the hour e.g. 10:25->10:30 instead of 10:27->10:32
for a 5 minute period.
@tommyers-elastic tommyers-elastic force-pushed the fix-cloudwatch-metric-interval-calculation branch from 41f45d9 to 19e7073 Compare August 17, 2022 13:02
@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 17, 2022 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-08-25T15:15:29.125+0000

  • Duration: 84 min 27 sec

Test stats 🧪

Test Results
Failed 0
Passed 1221
Skipped 107
Total 1328

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@tommyers-elastic tommyers-elastic marked this pull request as ready for review August 17, 2022 14:49
@tommyers-elastic tommyers-elastic requested a review from a team as a code owner August 17, 2022 14:49
kaiyan-sheng
kaiyan-sheng approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

Overall the change looks good to me. One question: adding now as an input parameter for GetStartTimeEndTime, is it to make the tests easier?

@kaiyan-sheng kaiyan-sheng added the Team:Cloud-Monitoring Label for the Cloud Monitoring team label Aug 22, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 22, 2022
@tommyers-elastic
Copy link
Contributor Author

Overall the change looks good to me. One question: adding now as an input parameter for GetStartTimeEndTime, is it to make the tests easier?

yep!

kaiyan-sheng
kaiyan-sheng approved these changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

Could you add a changelog for this one please? Thanks!!

@kaiyan-sheng kaiyan-sheng added backport-7.17 Automated backport to the 7.17 branch with mergify backport-v8.4.0 Automated backport with mergify labels Aug 25, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-cloudwatch-metric-interval-calculation upstream/fix-cloudwatch-metric-interval-calculation
git merge upstream/main
git push upstream fix-cloudwatch-metric-interval-calculation

@tommyers-elastic tommyers-elastic merged commit df3733e into elastic:main Aug 25, 2022
@mergify mergify bot mentioned this pull request Aug 25, 2022
Merged
mergify bot pushed a commit that referenced this pull request Aug 25, 2022
@tommyers-elastic @mergify
Fix a couple of bugs in the logic for how AWS metric periods are calc…
a6601a6 
…ulated (#32724)

* Fix a couple of bugs in the logic for how AWS metric periods are calcuated.

Firstly, we clarify that periods are always whole-minute durations.
Any period that is less than or in between minutes is rounded up to the
next whole-minute.

Secondly, we ensure that the resulting time period is always in the past.
This stops us getting empty metrics for the current-minute period.

Thirdly, we follow the AWS guidelines of aligning periods to whole
multiples within the hour e.g. 10:25->10:30 instead of 10:27->10:32
for a 5 minute period.

* add test that validates intervals are continuous for given periods

* update changelog

* fix merge error

(cherry picked from commit df3733e)
mergify bot pushed a commit that referenced this pull request Aug 25, 2022
@tommyers-elastic @mergify
Fix a couple of bugs in the logic for how AWS metric periods are calc…
c01321b 
…ulated (#32724)

* Fix a couple of bugs in the logic for how AWS metric periods are calcuated.

Firstly, we clarify that periods are always whole-minute durations.
Any period that is less than or in between minutes is rounded up to the
next whole-minute.

Secondly, we ensure that the resulting time period is always in the past.
This stops us getting empty metrics for the current-minute period.

Thirdly, we follow the AWS guidelines of aligning periods to whole
multiples within the hour e.g. 10:25->10:30 instead of 10:27->10:32
for a 5 minute period.

* add test that validates intervals are continuous for given periods

* update changelog

* fix merge error

(cherry picked from commit df3733e)
@mergify mergify bot mentioned this pull request Aug 25, 2022
Merged
kaiyan-sheng pushed a commit that referenced this pull request Aug 26, 2022
@mergify @tommyers-elastic
Fix a couple of bugs in the logic for how AWS metric periods are calc…
0a7a437 
…ulated (#32724) (#32872)

* Fix a couple of bugs in the logic for how AWS metric periods are calcuated.

Firstly, we clarify that periods are always whole-minute durations.
Any period that is less than or in between minutes is rounded up to the
next whole-minute.

Secondly, we ensure that the resulting time period is always in the past.
This stops us getting empty metrics for the current-minute period.

Thirdly, we follow the AWS guidelines of aligning periods to whole
multiples within the hour e.g. 10:25->10:30 instead of 10:27->10:32
for a 5 minute period.

* add test that validates intervals are continuous for given periods

* update changelog

* fix merge error

(cherry picked from commit df3733e)

Co-authored-by: Tom Myers <[email protected]>
kaiyan-sheng pushed a commit that referenced this pull request Aug 26, 2022
@mergify @tommyers-elastic
Fix a couple of bugs in the logic for how AWS metric periods are calc…
8426a55 
…ulated (#32724) (#32871)

* Fix a couple of bugs in the logic for how AWS metric periods are calcuated.

Firstly, we clarify that periods are always whole-minute durations.
Any period that is less than or in between minutes is rounded up to the
next whole-minute.

Secondly, we ensure that the resulting time period is always in the past.
This stops us getting empty metrics for the current-minute period.

Thirdly, we follow the AWS guidelines of aligning periods to whole
multiples within the hour e.g. 10:25->10:30 instead of 10:27->10:32
for a 5 minute period.

* add test that validates intervals are continuous for given periods

* update changelog

* fix merge error

(cherry picked from commit df3733e)

Co-authored-by: Tom Myers <[email protected]>
v1v added a commit to v1v/beats that referenced this pull request Sep 1, 2022
@v1v
Merge remote-tracking branch 'upstream/main' into feature/heartbeat-p…
a884d74 
…ackaging

* upstream/main: (109 commits)
  Add cap_net_raw requirements to heartbeat docs (elastic#32816)
  apply a quick hotfix for having main working properly (elastic#32934)
  action: checks for x-pack/libbeat and libbeat (elastic#32754)
  Update to Go 1.18 in go.mod. (elastic#32940)
  [heartbeat] disable browser code on windows via build tags (elastic#32939)
  action: checks for heartbeat and x-pack/heartbeat (elastic#32749)
  Make event acknowledgment asynchronous in shipper output (elastic#32785)
  [Automation] Update elastic stack version to 8.5.0-fedc3e60 for testing (elastic#32930)
  Preallocate memory to reduce GC load (elastic#32905)
  [Automation] Update elastic stack version to 8.5.0-440e0896 for testing (elastic#32919)
  Skip broken ceph tests. (elastic#32912)
  Use non-deprecated docker image for testing jolokia (elastic#32885)
  update ironbank image product name (elastic#32867)
  ci: pre-commit stage within Jenkins (elastic#32839)
  Fix a couple of bugs in the logic for how AWS metric periods are calculated (elastic#32724)
  [Filebeat] [httpjson] Add support for single string containing multiple relation-types in getRFC5988Link (elastic#32811)
  [Heartbeat] Update HB k8s template to use <Mi> metric (elastic#32801)
  action: checks for metricbeat and x-pack/metricbeat (elastic#32748)
  action: checks for filebeat and x-pack/filebeat (elastic#32746)
  allow for json/ndjson content type with charset (elastic#32767)
  ...
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
@tommyers-elastic @chrisberkhout
Fix a couple of bugs in the logic for how AWS metric periods are calc…
7c071ea 
…ulated (#32724)

* Fix a couple of bugs in the logic for how AWS metric periods are calcuated.

Firstly, we clarify that periods are always whole-minute durations.
Any period that is less than or in between minutes is rounded up to the
next whole-minute.

Secondly, we ensure that the resulting time period is always in the past.
This stops us getting empty metrics for the current-minute period.

Thirdly, we follow the AWS guidelines of aligning periods to whole
multiples within the hour e.g. 10:25->10:30 instead of 10:27->10:32
for a 5 minute period.

* add test that validates intervals are continuous for given periods

* update changelog

* fix merge error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaiyan-sheng kaiyan-sheng kaiyan-sheng approved these changes

Assignees

@tommyers-elastic tommyers-elastic

Labels
backport-7.17 Automated backport to the 7.17 branch with mergify backport-v8.4.0 Automated backport with mergify Team:Cloud-Monitoring Label for the Cloud Monitoring team
Projects
None yet
Milestone
No milestone
3 participants
@tommyers-elastic @elasticmachine @kaiyan-sheng