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

Add asPercent function #966

Merged
merged 33 commits into from
Aug 14, 2018
+1,592 −82
Merged

Add asPercent function #966

Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
58f3c4b
wip: Initial asPercent function. works with no arguments
stivenbb Jul 12, 2018
e32031f
added arg that accepts different types of args
stivenbb Jul 16, 2018
c434121
Bug fix: make ArgIn work properly with series AND allow series to be …
stivenbb Jul 18, 2018
cf3b879
fixed nodes parameter and added tests
stivenbb Jul 24, 2018
c01bb11
more tests
stivenbb Jul 25, 2018
79aeebe
modified readme
stivenbb Jul 25, 2018
6d8ae12
made requested changes
stivenbb Jul 25, 2018
1a17f36
changed comment
stivenbb Jul 25, 2018
2b147d2
removed unnecessary _
stivenbb Jul 26, 2018
8ff0653
sort output series in tests
stivenbb Jul 26, 2018
653c0ba
ensure that series don't get modified and added tests
stivenbb Jul 27, 2018
79d81bd
copy tags as well
stivenbb Jul 27, 2018
f5ed45b
split up Exec
stivenbb Aug 7, 2018
d690235
unit tests for ArgIn (based on asPercent)
Dieterbe Jun 27, 2017
863a454
Added Series.Copy; Fixed ArgIn kwarg error; changed behavior for empt…
stivenbb Aug 7, 2018
c90b4ac
copy only where necessary.
stivenbb Aug 8, 2018
f66913c
add old unit tests
Dieterbe Aug 9, 2018
36392d1
PR changes
stivenbb Aug 9, 2018
63c4513
formatting
stivenbb Aug 9, 2018
8fd0832
removed unused
stivenbb Aug 9, 2018
309c999
check if None is used for non-optional argument
stivenbb Aug 9, 2018
cc24e21
skip arg if optional None
stivenbb Aug 9, 2018
9171b99
it shouldn't be up to caller of consumeKwarg to provide half-consumed…
Dieterbe Aug 9, 2018
bd84f57
alphabetical ordering of cases
Dieterbe Aug 9, 2018
b476724
PR changes
stivenbb Aug 13, 2018
d350e37
format
stivenbb Aug 13, 2018
c480969
refactored tests and merged into one file
stivenbb Aug 13, 2018
172dccb
move pool recycling close to pool getting + bugfix
Dieterbe Aug 14, 2018
b6abd38
can use pool to source none slice
Dieterbe Aug 14, 2018
54afde0
better comments
Dieterbe Aug 14, 2018
22870b9
fix scoping issue
stivenbb Aug 14, 2018
2b34f46
don't sum unneeded totals
stivenbb Aug 14, 2018
7555e2b
Merge branch 'master' into asPercent
Dieterbe Aug 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
changed comment
@stivenbb
stivenbb committed Aug 7, 2018
commit 1a17f367a11712031f528b7ebaf2d63684802573
2 changes: 1 addition & 1 deletion expr/func_aspercent.go
View file
Original file line number Diff line number Diff line change
@@ -60,7 +60,7 @@ func (s *FuncAsPercent) Exec(cache map[Req][]models.Series) ([]models.Series, er
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same problem as before AFAICT. totals can be zero length, in which case we should return an error here.
or am i missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If totals is zero length, it is not an error. Instead, it will return a bunch of empty series with names something like asPercent(foo.*,MISSING)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh I see what you mean. Because the totalsSeries has to be either == 1 or == len(series)

But the error will get triggered at a later check. Plus, if the series is empty as well, then it should trigger an error technically

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally I think it's better to return the error early when we can. So when reasoning about the code we have fewer special cases that complicate things further down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I moved up the validation like you suggested, so the check for proper length of totals is right after it. (Gonna push soon just looking at one more thing)

Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we just check the len of totals right after

                totals, err = s.totalSeries.Exec(cache)
                if err != nil {
                        return nil, err
                }

?

Copy link
Contributor

Choose a reason for hiding this comment

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

@stivenbb ^^ why can't we just return an error here if len(totals) is 0 ? this case still slips through into the if s.nodes != ni case for example

Copy link
Contributor Author

@stivenbb stivenbb Aug 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

When nodes != nil and len(totals) == 0, it shouldn't fail. Instead, all targets will be asPercent(foo,MISSING) (Which is what Graphite would do)

Copy link
Contributor

Choose a reason for hiding this comment

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

aha! ok thx


if s.nodes != nil {
// List of keys (implemented as map for speed improvement)
// Set of keys
keys := make(map[string]struct{})
// Series grouped by key
metaSeries := groupSeriesByKey(series, s.nodes, &keys)