Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Fix summarize crash #1170

Merged
merged 3 commits into from
Dec 21, 2018
Merged

Fix summarize crash #1170

merged 3 commits into from
Dec 21, 2018

Conversation

shanson7
Copy link
Collaborator

@shanson7 shanson7 commented Dec 7, 2018

User reported crashes while trying to use "count" function with summarize. Seems like it was just a misalignment with the validation and accessor. Added some tests to summarize to make sure no other supported functions have issues.

Fixes #1134

@Dieterbe
Copy link
Contributor

https://graphite.readthedocs.io/en/latest/functions.html#graphite.render.functions.summarize doesn't mention cnt, only count. does graphite support it without documenting it?

@DanCech
Copy link
Contributor

DanCech commented Dec 14, 2018

@Dieterbe
Copy link
Contributor

I think adding support for count to FromConsolidateBy is the right move here, but i don't see what we gain by allowing cnt (other than just in general being more lenient for no particular reason, but graphite doesn't allow it so perhaps we shouldn't either)

@shanson7
Copy link
Collaborator Author

Keeping cnt is a compatibility measure for consolidateBy since it's been allowed as "Bonus" for a while (looks like ~3 years).

But it looks like it could never be used, assuming everything was calling Validate first.

@Dieterbe
Copy link
Contributor

Dieterbe commented Dec 20, 2018

since it's been allowed as "Bonus" for a while (looks like ~3 years)

hmm this is based on what you saw in consolidation.FromConsolidateBy() ?

But it looks like it could never be used, assuming everything was calling Validate first.

yes we validate before invoking the function using IsConsolFunc which calls consolidation.Validate which doesn't allow cnt

@Dieterbe
Copy link
Contributor

Dieterbe commented Dec 20, 2018

currently, in metrictank, for every caller of consolidation.FromConsolidateBy(<foo>), <foo> has been validated via IsConsolFunc, so <foo> would never have the value cnt.

to improve our interop with graphite (e.g. queries that work on just MT should still work when switching to graphite), let's remove the allowance for cnt consolidation.FromConsolidateBy() which we now is never used anyway.

I am open to allowing cnt as "syntactic sugar" (that was the goal initially anyway), but we should only do it if it's also made its way into graphite

Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

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

thanks Sean!

@Dieterbe Dieterbe merged commit 58c4fb1 into grafana:master Dec 21, 2018
@Dieterbe Dieterbe added this to the vnext milestone Feb 11, 2019
@shanson7 shanson7 deleted the fixSummarizeCrash branch March 6, 2019 14:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants