-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add combine action #1506
Add combine action #1506
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1506 +/- ##
=======================================
Coverage 88.71% 88.71%
=======================================
Files 346 347 +1
Lines 16934 16997 +63
=======================================
+ Hits 15023 15079 +56
- Misses 1443 1450 +7
Partials 468 468
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
3e81cb5
to
e2f5a2b
Compare
e2f5a2b
to
dc66376
Compare
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
} | ||
|
||
groupedTimeseries := mtp.groupTimeseries(allTimeseries, len(combinedMetric.MetricDescriptor.LabelKeys)) | ||
aggregatedTimeseries := mtp.mergeTimeseries(groupedTimeseries, transform.AggregationType, combinedMetric.MetricDescriptor.Type) |
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.
Do you validate that transform.AggregationType
is set correctly? Similar to Action being one of 3 possible "enum" values.
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.
Also, should the function be named combineTimeseries
, or action "merge" for consistency?
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.
Do you validate that
transform.AggregationType
is set correctly? Similar to Action being one of 3 possible "enum" values.
The way config is validated in this component is a little strange at the moment and there is validation missing for quite a few important cases. For now, I've added the obvious missing validation: on AggregationType
& OperationAction
.
This component probably needs a future PR to separate config into different types for different actions.
Also, should the function be named
combineTimeseries
, or action "merge" for consistency?
I think this is okay. One aspect of the "combine" action is merging timeseries. Merging timeseries is also relevant for the aggregating labels & aggregating label values actions.
2441400
to
7f4a58a
Compare
…jaeger (#1506) * Bump google.golang.org/api in /exporters/trace/jaeger Bumps [google.golang.org/api](https://github.com/googleapis/google-api-go-client) from 0.37.0 to 0.38.0. - [Release notes](https://github.com/googleapis/google-api-go-client/releases) - [Changelog](https://github.com/googleapis/google-api-go-client/blob/master/CHANGES.md) - [Commits](googleapis/google-api-go-client@v0.37.0...v0.38.0) Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
Description:
Add a "combine" action to the Metrics Transform processor:
See README & added test cases for more details.
Resolves #1088