This repository has been archived by the owner on Aug 23, 2023. It is now read-only.
Exp2 exp3 exp refactor #637
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
by using a shim of smartSummarize, which also introduces bool support
so that functions can express that they only want an integer, and so that they don't have to deal with verifying the float has no decimals, conversion to int, etc.
which requires a new argType "integers" and utility function extractMetric
* provide more extensive argument types, which can take care of setting
the right values, so the function just has to declare them
* add an elegant validation system, removing Init() from functions
* merge optional and mandatory arguments into one list for convenience
* allow specifying argument names for mandatory args as well. 2 reasons:
1) nice to mention name when showing an error for an invalid arg
2) consider this from graphite docs:
smartSummarize(seriesList, intervalString, func='sum',
alignToFrom=False)
-> graphite conflates name and type. we can name it interval with
type string
* make functions stateful, so we don't need to copy / redo same work
this saves us the hassle of coordinating the pipeline, getting outputs, feeding them from level into level, etc. also Exec can just return a []models.Series, that should be ok I think
graphite only allows a single, but why not, this is convienent
* make them methods of expr * rename silly j variable to pos, and k to key * handle seenKwargs stuff in caller
not sure why the tests didn't trip on this prior to the refactor :?
Dieterbe
force-pushed
the
exp2-exp3-exp-refactor
branch
from
a7c6954
to
12830b0
Compare
woodsaj
suggested changes
May 12, 2017
expr/funcs.go
Outdated
| float // number potentially with decimals | ||
| str // string | ||
| ) | ||
|
|
||
| type Func interface { |
There was a problem hiding this comment.
this still needs to be renamed. You already refer to these functions as GraphiteFuncs https://github.com/raintank/metrictank/blob/12830b0b3b025bc5f4e66e7252ddab9c73fe4637/expr/types.go#L4
woodsaj
approved these changes
May 12, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
replaces #622 #628 #631(and #625), i.e. expr2, expr3 and refactoring to make the planning/execution cleaner, which got messy after expr2.