Add Derivative Aggregator Plugin #3762
Conversation
|
@KarstenSchnitter are you still interested in getting this PR merged? If so, please make sure you have a CLA signed and indicate this in the PR-message above. |
|
|
||
| func (d *Derivative) calculateDenominator(aggregate aggregate) float64 { | ||
| if variable, present := d.derivationVariableName(); present { | ||
| return aggregate.last.fields[variable] - aggregate.first.fields[variable] |
There was a problem hiding this comment.
You need to make sure variable is actually contained in the fields!
There was a problem hiding this comment.
This one is still open see the discussion above.
| if denominator := d.calculateDenominator(aggregate); denominator != 0 { | ||
| derivatives := make(map[string]interface{}) | ||
| for key, start := range aggregate.first.fields { | ||
| if end, ok := aggregate.last.fields[key]; key != d.variableFieldName() && ok { |
There was a problem hiding this comment.
Split this in two ifs starting with skipping the denominator field.
Calculate derivatives based on time or fields.
* rebase to _master_ * improved documentation * introduced `Init()` function * check `uint64` in `convert` * do not copy `target` in `upsertConvertedFields`
I would always only update first in this case and never replace the metric. But I'm not sure if this is ok in your use-case as you might keep very old fields this way and to circumvent that problem you would need another structure with a per-field roll-over counter. So either you change to a per-metric-and-field |
KarstenSchnitter
force-pushed
the
aggregator_derivative
branch
from
29cdffc
to
fe293c1
Compare
|
I fixed a timestamp issue in the tests, that caused the failing checks. Now everything is green. Is there anything left to do? |
There was a problem hiding this comment.
I think we are very close. Please check my comments. Regarding the testing it might be an issue with the timestamp of your test values. You are using time.Now() which might have different resolutions on your machine and the CI machines. Just use some fixed timestamps to avoid any non-deterministic behavior. Otherwise just run your tests in a loop (e.g. 100 times) and check if it fails. We sometimes have those problems when e.g. accessing maps where access is shuffled by golang...
srebhan
added
plugin/aggregator
| time_difference | ||
| ``` | ||
| For each field the derivative is emitted with a naming pattern | ||
| `<fieldname>_by_time`. |
There was a problem hiding this comment.
how do you feel about _rate instead of _by_time?
There was a problem hiding this comment.
Actually the suffix is configurable. _by_time is currently the default value. I can change it to _rate if you prefer that.
| if current, ok := d.cache[id]; !ok { | ||
| // hit an uncached metric, create caches for first time: | ||
| d.cache[id] = newAggregate(in) | ||
| } else { |
There was a problem hiding this comment.
just return and flatten the else. makes it easier to read
| var denominator float64 | ||
| if len(d.Variable) == 0 { | ||
| // If no derivation variable is known default to timestamp | ||
| denominator = aggregate.last.time.Sub(aggregate.first.time).Seconds() |
There was a problem hiding this comment.
can you get rid of the else by making this the default value and flipping the condition?
There was a problem hiding this comment.
I do not know if I understand your suggestion. Wouldn't this mean I would always calculate the time differences, even when the are not needed?
There was a problem hiding this comment.
The idea of @ssoroka is to do
denominator = aggregate.last.time.Sub(aggregate.first.time).Seconds()
if len(d.Variable) > 0 {instead of the if/else construct. However this means that you, in the non-default-case, do a superfluous computation of the time difference. Don't know if this matters...
There was a problem hiding this comment.
This is how I understood @ssoroka. I just wanted to make sure, that I really should always calculate the time difference, even when it is not needed. I will make that change.
Co-authored-by: Steven Soroka <ssoroka78@gmail.com>
Co-authored-by: Steven Soroka <ssoroka78@gmail.com>
|
Wow, great to see the merge. Thanks for all the help and effort. |
This comment states that *if* the variable option is used, the name of the resulting variable is generated differently. However, in practice the name of the resulting variable will always just use the (specified or default) suffix, regardless of whether the variable option is set. It seems this behavior and this comment were both already present when the plugin was first introduced in PR influxdata#3762 (commit 927d34f), probably a remnant of changes during PR discussion.
This comment states that *if* the variable option is used, the name of the resulting variable is generated differently. However, in practice the name of the resulting variable will always just use the (specified or default) suffix, regardless of whether the variable option is set. It seems this behavior and this comment were both already present when the plugin was first introduced in PR influxdata#3762 (commit 927d34f), probably a remnant of changes during PR discussion.
Calculate derivatives based on time or fields.
Calculate metrics derivatives based on timestamp or other fields.
Required for all PRs: