[coordinator] Add mapping rule config support for keeping metrics at different resolutions #2036

robskillington · 2019-11-03T14:43:38Z

What this PR does / why we need it:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

codecov · 2019-11-04T13:14:12Z

Codecov Report

Merging #2036 into master will decrease coverage by 11.3%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #2036      +/-   ##
=========================================
- Coverage    72.4%   61.1%   -11.4%     
=========================================
  Files        1007     995      -12     
  Lines       86647   85473    -1174     
=========================================
- Hits        62805   52243   -10562     
- Misses      19609   29272    +9663     
+ Partials     4233    3958     -275

Flag	Coverage Δ
#aggregator	`71.3% <ø> (+5.1%)`	⬆️
#cluster	`58.7% <ø> (+7%)`	⬆️
#collector	`57.4% <ø> (-5.6%)`	⬇️
#dbnode	`74.7% <ø> (+25.5%)`	⬆️
#m3em	`11.8% <ø> (-48.5%)`	⬇️
#m3ninx	`30.7% <0%> (-34.1%)`	⬇️
#m3nsch	`17.7% <0%> (-28.1%)`	⬇️
#metrics	`17.7% <0%> (-0.1%)`	⬇️
#msg	`74.7% <ø> (+12.6%)`	⬆️
#query	`56.3% <0%> (+5.7%)`	⬆️
#x	`59.8% <ø> (+9.8%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d49308...83727d9. Read the comment docs.

codecov · 2019-11-04T13:15:51Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ec5ee86). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #2036   +/-   ##
========================================
  Coverage          ?   72.2%           
========================================
  Files             ?    1015           
  Lines             ?   87399           
  Branches          ?       0           
========================================
  Hits              ?   63186           
  Misses            ?   19979           
  Partials          ?    4234

Flag	Coverage Δ
#aggregator	`82% <ø> (?)`
#cluster	`85.6% <ø> (?)`
#collector	`64.8% <ø> (?)`
#dbnode	`79.7% <ø> (?)`
#m3em	`73.2% <ø> (?)`
#m3ninx	`73.9% <ø> (?)`
#m3nsch	`51.1% <ø> (?)`
#metrics	`17.6% <ø> (?)`
#msg	`74.9% <ø> (?)`
#query	`68.2% <ø> (?)`
#x	`83.1% <ø> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec5ee86...c623742. Read the comment docs.

arnikola · 2019-11-04T18:16:35Z

src/cmd/services/m3coordinator/downsample/options.go

+}
+
+// Rule returns the mapping rule for the mapping rule configuration.
+func (r MappingRuleConfiguration) Rule() (view.MappingRule, error) {


nit: pull this out into separate file rather than in options?

Still might be a good idea (it may already be moved and my gh is cached or something though)

arnikola · 2019-11-04T18:17:40Z

src/cmd/services/m3coordinator/downsample/options.go

@@ -191,6 +207,287 @@ type Configuration struct {
 	BufferPastLimits []BufferPastLimitConfiguration `yaml:"bufferPastLimits"`
 }

+// RulesConfiguration is a set of rules configuration to use for downsampling.
+type RulesConfiguration struct {
+	// MappingRules are the mapping rules that sets retention and resolution


...that set retention

arnikola · 2019-11-04T18:17:53Z

src/cmd/services/m3coordinator/downsample/options.go

+	// for metrics given a filter to match metrics against.
+	MappingRules []MappingRuleConfiguration `yaml:"mappingRules"`
+
+	// RollupRules are the rollup rules that sets specific aggregations for sets


RollupRules are rollup rules...

arnikola · 2019-11-04T18:18:58Z

src/cmd/services/m3coordinator/downsample/options.go

+	// - "P9999"
+	Aggregations []aggregation.Type `yaml:"aggregations"`
+
+	// StoragePolicies is the retention/resolution storage policies to keep the


StoragePolicies are the retention/resolution storage policies at which to keep matched metrics.

arnikola · 2019-11-04T18:19:35Z

src/cmd/services/m3coordinator/downsample/options.go

+	// keeping them with a storage policy.
+	Drop bool `yaml:"drop"`
+
+	// Optional fields follows.


Optional fields follow.

arnikola · 2019-11-04T18:21:11Z

src/cmd/services/m3coordinator/downsample/options.go

+
+// StoragePolicy returns the corresponding storage policy.
+func (p StoragePolicyConfiguration) StoragePolicy() (policy.StoragePolicy, error) {
+	return policy.ParseStoragePolicy(p.Resolution.String() + ":" + p.Retention.String())


nit: maybe Sprintf instead of concat here?

arnikola · 2019-11-04T18:56:21Z

src/cmd/services/m3coordinator/downsample/options.go

+
+// StoragePolicies returns storage policies.
+func (p StoragePolicyConfigurations) StoragePolicies() (policy.StoragePolicies, error) {
+	var storagePolicies policy.StoragePolicies


nit: init with size

arnikola · 2019-11-04T18:57:26Z

src/cmd/services/m3coordinator/downsample/options.go

+
+// RollupRuleConfiguration is a rollup rule configuration.
+type RollupRuleConfiguration struct {
+	// Filter is a string separated filter of labe name to label value


Filter is a space separated filter of label name to label value glob patterns to which to filter the mapping rule.

arnikola · 2019-11-04T18:58:08Z

src/cmd/services/m3coordinator/downsample/options.go

+	// Transforms are a set of of rollup rule transforms.
+	Transforms []TransformConfiguration `yaml:"transforms"`
+
+	// StoragePolicies is the retention/resolution storage policies to keep the


StoragePolicies are the retention/resolution storage policies at which to keep the matched metrics

arnikola · 2019-11-04T18:58:18Z

src/cmd/services/m3coordinator/downsample/options.go

+	// matched metrics at.
+	StoragePolicies []StoragePolicyConfiguration `yaml:"storagePolicies"`
+
+	// Optional fields follows.


Optional fields follow.

arnikola · 2019-11-04T18:59:20Z

src/cmd/services/m3coordinator/downsample/options.go

+// RollupOperationConfiguration is a rollup operation.
+type RollupOperationConfiguration struct {
+	// MetricName is the name of the new metric that is emitted after
+	// the rollup is applied with it's aggregations and group by's.


with its aggregations and group by's.

arnikola · 2019-11-04T19:01:01Z

src/cmd/services/m3coordinator/downsample/options.go

+		return view.RollupRule{}, err
+	}
+
+	var ops []pipeline.OpUnion


Init with size, also is it worth doing a validation step to ensure that there's not multiple things set?

arnikola

Bunch of nits but nothing worth blocking for, approving if you want to address async

…ping rules

…m:m3db/m3 into r/mapping-rule-rollup-rule-config-support

anandsinghkunwar · 2020-01-02T15:40:36Z

@robskillington is this supposed to make the next release?

robskillington · 2020-01-02T15:44:57Z

@anandsinghkunwar yes, however we may end up only supporting mapping rules at first. Rollup rules may take some more time as there is some intricacies in performing rollups on metrics with pre-existing timestamps.

Mapping rules allows for selecting metrics to store in different namespaces selectively however and should be easy to merge and have supported for next release. e.g.:

downsample:
  rules:
    mappingRules:
      - name: "nginx metrics"
        filter: "app:nginx*"
        aggregations: ["Last"]
        storagePolicies:
          - resolution: 10m
            retention: 4320h # 180 days

arnikola · 2020-01-02T14:11:12Z

scripts/docker-integration-tests/run.sh

+	# scripts/docker-integration-tests/repair/test.sh
+	# scripts/docker-integration-tests/replication/test.sh
+	# scripts/docker-integration-tests/repair_and_replication/test.sh
+	# scripts/docker-integration-tests/multi_cluster_write/test.sh


We should uncomment these

arnikola · 2020-01-02T14:14:27Z

src/metrics/rules/store/kv/store.go

@@ -114,6 +114,9 @@ func (s *store) WriteAll(nss *rules.Namespaces, rs rules.MutableRuleSet) error {
 	if err != nil {
 		return err
 	}
+
+	r, _ := rs.RollupRules()
+	fmt.Println("ruleset rollup rules: ", r)


Delete this? Or actually might be better to replace with a debug level log?

arnikola · 2020-01-02T14:15:43Z

scripts/docker-integration-tests/coordinator_config_rules/m3coordinator.yml

+    # rollupRules:
+    #   - filter: "foo:bar"
+    #     transforms:
+    #       - rollup:
+    #           metricName: "new_metric"
+    #           aggregations:
+    #             - Sum
+    #           groupBy:
+    #             - foo
+    #         # aggregate:
+    #         #   type: Sum
+    #     storagePolicies:
+    #       - retention: 10h
+    #         resolution: 5s
+    #     name: "testRollup"


Should these be uncommented for the test?

arnikola · 2020-01-02T14:18:37Z

src/cmd/services/m3coordinator/downsample/options.go

@@ -170,6 +183,10 @@ type agg struct {

 // Configuration configurates a downsampler.
 type Configuration struct {
+	// Rules is a set of downsample rules, if this rules configuration is set


Might read a little clearer as // Rules is a set of downsample rules. If set, this overrides any rules set in the KV store.

arnikola · 2020-01-02T14:22:16Z

scripts/docker-integration-tests/coordinator_config_rules/test.sh

+  status=$(echo $out | grep -v promremotecli_log | docker run --rm -i $JQ_IMAGE jq .statusCode)
+  if [[ "$success" != "$expect_success" ]]; then
+    echo $expect_success_err
+    sleep 10000000


Should this be sleep 1? Otherwise it's ~115 days

arnikola · 2020-01-02T19:12:54Z

src/cmd/services/m3coordinator/downsample/options.go

+
+	ops := make([]pipeline.OpUnion, 0, len(r.Transforms))
+	for _, elem := range r.Transforms {
+		// TODO: make sure only one of "Rollup" or "Aggregate" or "Transform" is not nil


nit; This may be a bit cleaner as a method on TransformConfiguration?

arnikola · 2020-01-02T19:14:41Z

src/cmd/services/m3coordinator/downsample/options.go

+					NewName:          cfg.MetricName,
+					Tags:             cfg.GroupBy,


Should this ensure that MetricName is not in GroupBy?

Should be ok.

arnikola · 2020-01-02T19:15:01Z

src/cmd/services/m3coordinator/downsample/options.go

+
+// TransformOperationConfiguration is a transform operation.
+type TransformOperationConfiguration struct {
+	// Type is an transformation operation type.


nit: a transformation...

arnikola · 2020-01-02T19:19:14Z

src/cmd/services/m3coordinator/downsample/options.go

+func (c *aggregatorLocalAdminClient) SetAggregator(agg aggregator.Aggregator) {
+	c.agg = agg
+}


Can probably roll this into newAggregatorLocalAdminClient ? Also doesn't seem to need exporting

I unexported it but kept it since // NB: Can't do this at construction time since needs to be passed as an // option to the aggregator constructor.

arnikola · 2020-01-02T19:39:59Z

src/cmd/services/m3coordinator/downsample/options.go

+		SetRuleSetOptions(ruleSetOpts).
+		SetKVStore(rulesKVStore)
+
+	// NB(r): If rules are being explicitlly set in config then we are


nit; explicitlly -> explicitly

arnikola · 2020-01-02T20:29:53Z

scripts/docker-integration-tests/coordinator_config_rules/test.sh

+    true "Expected request to succeed" \
+    200 "Expected request to return status code 200"
+
+  start=$(expr $(date +"%s") - 3600)


nit: define end first, then subtract from that, just to avoid any potential issues with crossing any boundaries?

arnikola · 2020-01-02T22:22:22Z

BTW does this need to set namespace ClusterNamespaceDownsampleOptions.All to false for any not matching namespaces? Otherwise query fanout may miss points

…m:m3db/m3 into r/mapping-rule-rollup-rule-config-support

robskillington added 3 commits November 3, 2019 15:42

Add mapping rule and rollup rule config support

2d0a0a4

Add config ability

83727d9

Merge branch 'master' into r/mapping-rule-rollup-rule-config-support

c5022de

arnikola reviewed Nov 4, 2019

View reviewed changes

arnikola approved these changes Nov 4, 2019

View reviewed changes

robskillington and others added 9 commits November 14, 2019 09:24

Address feedback

0017ede

Add mapping rule and rollup rule config support

7f8862e

Add config ability

863851a

Address feedback

72c5683

Fix downsampler not using in mem store if using config for rollup/map…

25c4760

…ping rules

[WIP] Add tests

4fe4c06

Create namespace first

033dc0b

Merge branch 'r/mapping-rule-rollup-rule-config-support' of github.co…

7a50808

…m:m3db/m3 into r/mapping-rule-rollup-rule-config-support

Add unit tests and update integration test

2da1b2f

robskillington force-pushed the r/mapping-rule-rollup-rule-config-support branch from 5f7bc6b to 2da1b2f Compare January 2, 2020 15:39

robskillington changed the title ~~Add mapping rule and rollup rule config support~~ [coordinator] Add mapping rule config support for keeping metrics at different resolutions Jan 2, 2020

robskillington added 6 commits January 2, 2020 10:55

Restore integration test runs

8fc40db

Merge branch 'master' into r/mapping-rule-rollup-rule-config-support

827d58d

Fix tests

06ebbbe

Use Go 1.13 for docker integration tests

11c654d

Address feedback

4d27c03

Fix codegen CI failures

1c65e49

arnikola reviewed Jan 2, 2020

View reviewed changes

benraskin92 and others added 9 commits January 3, 2020 10:37

Add long namespaces to integration test

9824631

Merge branch 'r/mapping-rule-rollup-rule-config-support' of github.co…

6bdfc23

…m:m3db/m3 into r/mapping-rule-rollup-rule-config-support

Remove imports from restrict.go

5a4ffd2

Update end time and jq

ed335f4

Merge branch 'r/mapping-rule-rollup-rule-config-support' of github.co…

ec5ee86

…m:m3db/m3 into r/mapping-rule-rollup-rule-config-support

Merge branch 'master' into r/mapping-rule-rollup-rule-config-support

0f1e48f

Updates

4476d55

Merge branch 'r/mapping-rule-rollup-rule-config-support' of github.co…

33cdac6

…m:m3db/m3 into r/mapping-rule-rollup-rule-config-support

Address feedback

c623742

robskillington merged commit a24c337 into master Jan 3, 2020

robskillington deleted the r/mapping-rule-rollup-rule-config-support branch January 3, 2020 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[coordinator] Add mapping rule config support for keeping metrics at different resolutions #2036

[coordinator] Add mapping rule config support for keeping metrics at different resolutions #2036

robskillington commented Nov 3, 2019

codecov bot commented Nov 4, 2019

codecov bot commented Nov 4, 2019 •

edited

Loading

arnikola Nov 4, 2019

robskillington Nov 4, 2019

arnikola Nov 14, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola Nov 4, 2019

arnikola left a comment

anandsinghkunwar commented Jan 2, 2020

robskillington commented Jan 2, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

robskillington Jan 3, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

robskillington Jan 3, 2020

arnikola Jan 2, 2020

arnikola Jan 2, 2020

arnikola commented Jan 2, 2020

[coordinator] Add mapping rule config support for keeping metrics at different resolutions #2036

[coordinator] Add mapping rule config support for keeping metrics at different resolutions #2036

Conversation

robskillington commented Nov 3, 2019

codecov bot commented Nov 4, 2019

Codecov Report

codecov bot commented Nov 4, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

anandsinghkunwar commented Jan 2, 2020

robskillington commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola commented Jan 2, 2020

codecov bot commented Nov 4, 2019 •

edited

Loading