Skip to content
This repository was archived by the owner on Nov 8, 2022. It is now read-only.

Commit 92e2486

Browse files
jcooklinjcooklin
committed
Fixes #1228 - metric request ambiguous
* Updates validateMetric on subscriptionGroups to use GetMetrics instead of GetMetric since some namespace requests can expand to include multiple metrics.
1 parent 068def9 commit 92e2486

8 files changed

Lines changed: 201 additions & 157 deletions

File tree

‎control/control_test.go‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -815,6 +815,9 @@ func TestMetricConfig(t *testing.T) {
815815
Convey("So metric should be valid with config", func() {
816816
errs := c.subscriptionGroups.validateMetric(m1)
817817
So(errs, ShouldBeNil)
818+
Convey("So mock should have name: bob config from defaults", func() {
819+
So(c.Config.Plugins.pluginCache["0"+core.Separator+"mock"+core.Separator+"1"].Table()["name"], ShouldResemble, ctypes.ConfigValueStr{Value: "bob"})
820+
})
818821
})
819822

820823
c.Stop()
@@ -1047,10 +1050,10 @@ func TestCollectDynamicMetrics(t *testing.T) {
10471050
<-lpe.done
10481051
metrics, err := c.metricCatalog.Fetch(core.NewNamespace())
10491052
So(err, ShouldBeNil)
1050-
So(len(metrics), ShouldEqual, 6)
1053+
So(len(metrics), ShouldEqual, 8)
10511054
mts, err := c.metricCatalog.GetMetrics(core.NewNamespace("intel", "mock", "*", "baz"), 2)
10521055
So(err, ShouldBeNil)
1053-
So(len(mts), ShouldEqual, 1)
1056+
So(len(mts), ShouldEqual, 2)
10541057
m := mts[0]
10551058
errs := c.subscriptionGroups.validateMetric(m)
10561059
So(errs, ShouldBeNil)
@@ -1093,7 +1096,7 @@ func TestCollectDynamicMetrics(t *testing.T) {
10931096
So(err, ShouldBeNil)
10941097
So(hits, ShouldEqual, 0)
10951098
So(errs, ShouldBeNil)
1096-
So(len(mts), ShouldEqual, 10)
1099+
So(len(mts), ShouldEqual, 11)
10971100
mts, errs = c.CollectMetrics(taskID, nil)
10981101
hits, err = pool.CacheHits(m.namespace.String(), 2, taskID)
10991102
So(err, ShouldBeNil)
@@ -1102,7 +1105,7 @@ func TestCollectDynamicMetrics(t *testing.T) {
11021105
// So(hits, ShouldEqual, 1)
11031106

11041107
So(errs, ShouldBeNil)
1105-
So(len(mts), ShouldEqual, 10)
1108+
So(len(mts), ShouldEqual, 11)
11061109
pool.Unsubscribe(taskID)
11071110
pool.SelectAndKill(taskID, "unsubscription event")
11081111
So(pool.Count(), ShouldEqual, 0)
@@ -1212,7 +1215,7 @@ func TestCollectMetrics(t *testing.T) {
12121215
<-lpe.done
12131216
mts, err := c.MetricCatalog()
12141217
So(err, ShouldBeNil)
1215-
So(len(mts), ShouldEqual, 4)
1218+
So(len(mts), ShouldEqual, 5)
12161219

12171220
cd := cdata.NewNode()
12181221
cd.AddItem("password", ctypes.ConfigValueStr{Value: "testval"})
@@ -1306,7 +1309,7 @@ func TestCollectNonSpecifiedDynamicMetrics(t *testing.T) {
13061309
<-lpe.done
13071310
mts, err := c.MetricCatalog()
13081311
So(err, ShouldBeNil)
1309-
So(len(mts), ShouldEqual, 4)
1312+
So(len(mts), ShouldEqual, 5)
13101313

13111314
cd := cdata.NewNode()
13121315

@@ -1343,16 +1346,19 @@ func TestCollectNonSpecifiedDynamicMetrics(t *testing.T) {
13431346
So(len(mts), ShouldBeGreaterThan, len(requested))
13441347
// expected 10 metrics "/intel/mock/[host_id]/baz
13451348
// for hosts in range (0 - 9)
1346-
So(len(mts), ShouldEqual, 10)
1349+
So(len(mts), ShouldEqual, 11)
13471350
for _, m := range mts {
13481351
// ensure the collected metric's namespace starts with /intel/mock/host...
1349-
So(m.Namespace().String(), ShouldStartWith, core.NewNamespace("intel", "mock", "host").String())
1352+
So(m.Namespace().String(), ShouldStartWith, core.NewNamespace("intel", "mock").String())
1353+
So(m.Namespace().String(), ShouldContainSubstring, "baz")
13501354

13511355
// ensure the collected data coming back is from v1
13521356
So(m.Version(), ShouldEqual, 1)
13531357
// ensure the collected data is dynamic
1354-
isDynamic, _ := m.Namespace().IsDynamic()
1355-
So(isDynamic, ShouldBeTrue)
1358+
if !strings.Contains(m.Namespace().String(), "all") {
1359+
isDynamic, _ := m.Namespace().IsDynamic()
1360+
So(isDynamic, ShouldBeTrue)
1361+
}
13561362
}
13571363
}
13581364

@@ -1386,9 +1392,9 @@ func TestCollectSpecifiedDynamicMetrics(t *testing.T) {
13861392

13871393
mts, err := c.MetricCatalog()
13881394
So(err, ShouldBeNil)
1389-
// metric catalog should contain the 3 following metrics:
1390-
// /intel/mock/foo; /intel/mock/bar; /intel/mock/*/baz
1391-
So(len(mts), ShouldEqual, 3)
1395+
// metric catalog should contain the 4 following metrics:
1396+
// /intel/mock/foo; /intel/mock/bar; /intel/mock/*/baz; /intel/mock/all/baz
1397+
So(len(mts), ShouldEqual, 4)
13921398

13931399
Convey("collection for specified host id - positive", func() {
13941400
taskID := "task-01"
@@ -1988,7 +1994,9 @@ func TestDynamicMetricSubscriptionLoadLessMetrics(t *testing.T) {
19881994
Convey("metrics are collected from mock1 and mock2", func() {
19891995
// ensure the data coming back is from mock 1 and mock 2
19901996
for _, m := range mts2 {
1991-
if strings.Contains(m.Namespace().String(), "host") || strings.Contains(m.Namespace().String(), "bar") {
1997+
if strings.Contains(m.Namespace().String(), "host") ||
1998+
strings.Contains(m.Namespace().String(), "bar") ||
1999+
strings.Contains(m.Namespace().String(), "all") {
19922000
val, ok := m.Data().(int)
19932001
So(ok, ShouldEqual, true)
19942002
So(val, ShouldBeGreaterThan, 1000)

‎control/subscription_group.go‎

Lines changed: 44 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -273,62 +273,65 @@ func (p *subscriptionGroups) validatePluginSubscription(pl core.SubscribedPlugin
273273

274274
func (s *subscriptionGroups) validateMetric(
275275
metric core.Metric) (serrs []serror.SnapError) {
276-
m, err := s.metricCatalog.GetMetric(metric.Namespace(), metric.Version())
276+
mts, err := s.metricCatalog.GetMetrics(metric.Namespace(), metric.Version())
277277
if err != nil {
278278
serrs = append(serrs, serror.New(err, map[string]interface{}{
279279
"name": metric.Namespace().String(),
280280
"version": metric.Version(),
281281
}))
282282
return serrs
283283
}
284+
for _, m := range mts {
284285

285-
// No metric found return error.
286-
if m == nil {
287-
serrs = append(
288-
serrs, serror.New(
289-
fmt.Errorf("no metric found cannot subscribe: (%s) version(%d)",
290-
metric.Namespace(), metric.Version())))
291-
return serrs
292-
}
286+
// No metric found return error.
287+
if m == nil {
288+
serrs = append(
289+
serrs, serror.New(
290+
fmt.Errorf("no metric found cannot subscribe: (%s) version(%d)",
291+
metric.Namespace(), metric.Version())))
292+
continue
293+
}
293294

294-
m.config = metric.Config()
295+
m.config = metric.Config()
295296

296-
typ, serr := core.ToPluginType(m.Plugin.TypeName())
297-
if serr != nil {
298-
return []serror.SnapError{serror.New(err)}
299-
}
297+
typ, serr := core.ToPluginType(m.Plugin.TypeName())
298+
if serr != nil {
299+
serrs = append(serrs, serror.New(err))
300+
continue
301+
}
300302

301-
// merge global plugin config
302-
if m.config != nil {
303-
m.config.ReverseMergeInPlace(
304-
s.Config.Plugins.getPluginConfigDataNode(typ,
305-
m.Plugin.Name(), m.Plugin.Version()))
306-
} else {
307-
m.config = s.Config.Plugins.getPluginConfigDataNode(typ,
308-
m.Plugin.Name(), m.Plugin.Version())
309-
}
303+
// merge global plugin config
304+
if m.config != nil {
305+
m.config.ReverseMergeInPlace(
306+
s.Config.Plugins.getPluginConfigDataNode(typ,
307+
m.Plugin.Name(), m.Plugin.Version()))
308+
} else {
309+
m.config = s.Config.Plugins.getPluginConfigDataNode(typ,
310+
m.Plugin.Name(), m.Plugin.Version())
311+
}
310312

311-
// When a metric is added to the MetricCatalog, the policy of rules defined by the plugin is added to the metric's policy.
312-
// If no rules are defined for a metric, we set the metric's policy to an empty ConfigPolicyNode.
313-
// Checking m.policy for nil will not work, we need to check if rules are nil.
314-
if m.policy.HasRules() {
315-
if m.Config() == nil {
316-
fields := log.Fields{
317-
"metric": m.Namespace(),
318-
"version": m.Version(),
319-
"plugin": m.Plugin.Name(),
313+
// When a metric is added to the MetricCatalog, the policy of rules defined by the plugin is added to the metric's policy.
314+
// If no rules are defined for a metric, we set the metric's policy to an empty ConfigPolicyNode.
315+
// Checking m.policy for nil will not work, we need to check if rules are nil.
316+
if m.policy.HasRules() {
317+
if m.Config() == nil {
318+
fields := log.Fields{
319+
"metric": m.Namespace(),
320+
"version": m.Version(),
321+
"plugin": m.Plugin.Name(),
322+
}
323+
serrs = append(serrs, serror.New(ErrConfigRequiredForMetric, fields))
324+
continue
320325
}
321-
serrs = append(serrs, serror.New(ErrConfigRequiredForMetric, fields))
322-
return serrs
323-
}
324-
ncdTable, errs := m.policy.Process(m.Config().Table())
325-
if errs != nil && errs.HasErrors() {
326-
for _, e := range errs.Errors() {
327-
serrs = append(serrs, serror.New(e))
326+
ncdTable, errs := m.policy.Process(m.Config().Table())
327+
if errs != nil && errs.HasErrors() {
328+
for _, e := range errs.Errors() {
329+
serrs = append(serrs, serror.New(e))
330+
}
331+
continue
328332
}
329-
return serrs
333+
m.config = cdata.FromTable(*ncdTable)
330334
}
331-
m.config = cdata.FromTable(*ncdTable)
332335
}
333336

334337
return serrs

0 commit comments

Comments
 (0)