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

Align metrics windows to gather interval for cloudwatch input #4667

Merged
merged 1 commit into from
Sep 11, 2018

Conversation

jcmcken
Copy link
Contributor

@jcmcken jcmcken commented Sep 10, 2018

Fixes #4643

For consideration, please review

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@jcmcken
Copy link
Contributor Author

jcmcken commented Sep 10, 2018

There didn't seem to be an obvious place that I should update the README, so I made no changes.

@glinton glinton added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 10, 2018
@glinton glinton changed the title Align metrics windows to gather interval Align metrics windows to gather interval for cloudwatch input Sep 10, 2018
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Judging by the current code, it's not required to round the start-time/end-time?


windowEnd := relativeTo.Add(-c.Delay.Duration)

if c.windowEnd == defaultTime {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: if c.windowEnd.IsZero()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know that function was there. I'll update

As for the rounding, is there something you had in mind? It looks like, based on this that non-custom metrics have a minimum period of 60s. It also says:

When statistics are aggregated over a period of time, they are stamped with the time corresponding to the beginning of the period.

Seems to indicate it should be aligned to the start of the minute. Is this what you're referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

On the rounding, it's probably not something that needs done as I don't see us doing it before. I was just curious if the times needed to rounded and aligned, but I guess either of these ranges would work fine (second one is not aligned to period) and what really matters is that we cover all times?

--start-time 2018-09-06T17:15:00Z --end-time 2018-09-06T17:30:00Z --period 300
--start-time 2018-09-06T17:16:42Z --end-time 2018-09-06T17:31:42Z --period 300

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the zero function.

I think we probably don't need to align the data as long as we cover all time periods, like you said.

@danielnelson danielnelson added this to the 1.8.0 milestone Sep 10, 2018
@danielnelson danielnelson merged commit 03a119e into influxdata:master Sep 11, 2018
rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants