-
Notifications
You must be signed in to change notification settings - Fork 107
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did an internal review of this implementation already. It has been running on our production cluster for a couple days and is quite an improvement in speed.
---------- Native Implementation ----------
Requests [total, rate] 1200, 10.01
Duration [total, attack, wait] 1m59.929790104s, 1m59.899999891s, 29.790213ms
Latencies [mean, 50, 95, 99, max] 29.710889ms, 25.292608ms, 42.891724ms, 124.23679ms, 530.666495ms
Bytes In [total, mean] 13865883, 11554.90
Bytes Out [total, mean] 0, 0.00
Success [ratio] 100.00%
Status Codes [code:count] 200:1200
Error Set:
---------- Graphite (Python) Implementation ----------
Requests [total, rate] 1200, 10.01
Duration [total, attack, wait] 2m0.256803282s, 1m59.899999886s, 356.803396ms
Latencies [mean, 50, 95, 99, max] 183.917679ms, 109.067328ms, 559.073411ms, 792.026173ms, 1.12680363s
Bytes In [total, mean] 33578816, 27982.35
Bytes Out [total, mean] 0, 0.00
Success [ratio] 100.00%
Status Codes [code:count] 200:1200
Error Set:
On average, the native implementation was 6x faster, median was 4x faster, p95 was 13x faster, p99 was 6x faster and max was over 2x faster
expr/func_isnonnull.go
Outdated
|
||
out := make([]models.Series, 0, len(series)) | ||
for _, serie := range series { | ||
transformed := models.Series{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the out
slice is already allocated with the correct size. wouldn't it be better to just update its properties instead of instantiating a new models.Series
and then copying it into out
?
f.e.:
transformed := &out[i]
transformed.Target = fmt.Sprintf("isNonNull(%s)", serie.Target)
(i
is the iterator of the loop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with one suggestion
Added requested change |
GitHub seems to be broken right now. I clicked the "Merge" button which ended up in an error page. Looking at the |
@replay mind trying again to accordingly update the PR page? |
@stivenbb I'm afraid I'll only be able to just close this PR. As the commits are merged already, I guess that shouldn't make a difference: |
I see the commits in there, and @stivenbb gets credit as a contributor :) I think closing this is fine. |
Add isNonNull function
Implementation of isNonNull() graphite function.
Takes a seriesList and counts up non-null values. Returns a seriesList where 1 is specified for non-null values, and 0 is specified for null values.