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

[query] Update Graphite consolidateBy function to match original implementation for maxDataPoints and "last" aggregation #3602

Merged
merged 5 commits into from
Jul 13, 2021
Merged

[query] Update Graphite consolidateBy function to match original implementation for maxDataPoints and "last" aggregation #3602

merged 5 commits into from
Jul 13, 2021

Conversation

robskillington
Copy link
Collaborator

What this PR does / why we need it:

This updates the Graphite consolidateBy function to resize the output when max datapoints is specified (rather than leaving it up to final output rendering to choose consolidation approach).

It also adds the missing "last" consolidation approach for consolidateBy.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

benraskin92
benraskin92 approved these changes Jul 13, 2021
View reviewed changes
Copy link
Collaborator

@benraskin92 benraskin92 left a comment

Choose a reason for hiding this comment

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

Lgtm

@robskillington robskillington enabled auto-merge (squash) July 13, 2021 19:13
yyin-sc
yyin-sc approved these changes Jul 13, 2021
View reviewed changes
Copy link
Collaborator

@yyin-sc yyin-sc left a comment

Choose a reason for hiding this comment

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

LGTM, one minor comment.

src/query/graphite/native/builtin_functions.go
newSeries := series.RenamedTo(newName)
newSeries.SetConsolidationFunc(cf)

// Check if needs to resized datapoints to fit max data points so that
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/resized/resize

@robskillington robskillington merged commit fcd0944 into master Jul 13, 2021
@robskillington robskillington deleted the r/fix-consolidate-by-resize-to-max-datapoints branch July 13, 2021 19:23
soundvibe added a commit that referenced this pull request Jul 14, 2021
@soundvibe
Merge branch 'master' into linasn/shard-close-seekers
7ab2dcd 
* master:
  [buildkite] Use alpine 3.13 to avoid issues with faccessat2 (#3603)
  [query] Update Graphite consolidateBy function to match original implementation for maxDataPoints and "last" aggregation (#3602)
  [buildkite] Setup actual go 1.16 docker image (#3601)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yyin-sc yyin-sc yyin-sc approved these changes

@benraskin92 benraskin92 benraskin92 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robskillington @benraskin92 @yyin-sc