Add aggregation capability to signalfx exporter translations #501
Conversation
dmitryax
force-pushed
the
aggregate-sfx-exporter
branch
3 times, most recently
from
9e7ddca
to
8566792
Compare
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
==========================================
+ Coverage 85.96% 86.03% +0.06%
==========================================
Files 187 187
Lines 10063 10142 +79
==========================================
+ Hits 8651 8726 +75
- Misses 1091 1093 +2
- Partials 321 323 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
dmitryax
force-pushed
the
aggregate-sfx-exporter
branch
from
8566792
to
e7e00f5
Compare
dmitryax
changed the title
dmitryax
force-pushed
the
aggregate-sfx-exporter
branch
from
e7e00f5
to
db56d8c
Compare
dmitryax
changed the title
dmitryax
force-pushed
the
aggregate-sfx-exporter
branch
2 times, most recently
from
7bc3b31
to
5b6a503
Compare
| // NOTE: Based on the usage of TranslateDataPoints we can assume that the datapoints batch []*sfxpb.DataPoint | ||
| // represents only one metric and all the datapoints can be aggregated together. |
There was a problem hiding this comment.
I don't understand the comment. I see that below we check the MetricName, so presumably it means the batch can contain different metrics?
There was a problem hiding this comment.
I put a comment in the check below. Different names can potentially go from copy_metric translation rules in the actions list before aggregation.
| for _, d := range dp.Dimensions { | ||
| if d.Key == dimensionKey { | ||
| if _, ok := dimValToDps[d.Value]; !ok { | ||
| dimValToDps[d.Value] = make([]*sfxpb.DataPoint, 0, len(dps)-i) |
There was a problem hiding this comment.
It would be worth to explain why len(dps)-i is a good idea.
| result := make([]*sfxpb.DataPoint, 0, len(dimValToDps)) | ||
| for _, dps := range dimValToDps { | ||
| dp := proto.Clone(dps[0]).(*sfxpb.DataPoint) | ||
| dp.Dimensions = getCommonDimensions(dps) |
There was a problem hiding this comment.
Why do we need to calculate common dimensions? Aren't we reducing this metric to only contain the single dimension across which we aggregate?
There was a problem hiding this comment.
yes, this is a bit unusual for aggregations, but since we are getting a bunch of common dimensions from resource attributes like k8s.node.name, k8s.cluster.name etc, we would like to keep them as is.
There was a problem hiding this comment.
Instead , we could explicitly specify list of dimensions to aggregate across
There was a problem hiding this comment.
I'm changing it to use this approach ^ . Please do not review yet
There was a problem hiding this comment.
@tigrannajaryan I refactored it accordingly. PTAL
| @@ -65,6 +65,28 @@ const ( | |||
| // k8s.pod.network.io{direction="receive"} -> pod_network_receive_bytes_total{} | |||
| // k8s.pod.network.io{direction="transmit"} -> pod_network_transmit_bytes_total{} | |||
| ActionSplitMetric Action = "split_metric" | |||
|
|
|||
There was a problem hiding this comment.
This explanation is excellent. Just a couple of grammar nits
| @@ -266,6 +344,86 @@ func (mp *MetricTranslator) TranslateDimension(orig string) string { | |||
| return orig | |||
| } | |||
|
|
|||
| // aggregateDatapoints aggregates datapoints assuming that they have | |||
| // the same Timestamp, MetricType, Metric and Source fields. | |||
| func aggregateDatapoints( | |||
There was a problem hiding this comment.
Might be nice to have a unit test just for this fcn and getCommonDimensions but they're already covered, so totally up to you.
There was a problem hiding this comment.
Yea, I'd like to use the same test framework with defined input/output and get it covered through TranslateDataPoints
dmitryax
force-pushed
the
aggregate-sfx-exporter
branch
from
5b6a503
to
a7155e9
Compare
|
@tigrannajaryan @pmcollins comments addressed |
dmitryax
force-pushed
the
aggregate-sfx-exporter
branch
from
a7155e9
to
9a2bed0
Compare
We need simple aggregation capability to get machine_cpu_cores metric. For now only "count" aggregation method is needed
dmitryax
force-pushed
the
aggregate-sfx-exporter
branch
from
9a2bed0
to
fc3459a
Compare
Description:
We need simple aggregation capability to get machine_cpu_cores metric. For now only "count" aggregation method is needed. Later this might be moved to metrics transform processor.