Merge pull request #766 from tasdomas/add-metrics-limited-to-collect-…

…metrics-hook

Limit adding and sending metrics to the collect-metrics hook.

Now that the hook tool add-metrics has been introduced, its usage will be limited to the collect-metrics hook. At this point the hook is only defined, not actually executed.

cmd/juju/helptool.go

@@ -42,7 +42,6 @@ func (dummyHookContext) ConfigSettings() (charm.Settings, error) {
 func (dummyHookContext) ActionParams() map[string]interface{} {
 	return nil
 }
-
 func (dummyHookContext) HookRelation() (jujuc.ContextRelation, bool) {
 	return nil, false
 }

dependencies.tsv

@@ -20,7 +20,7 @@ github.com/juju/schema	git	27a52be50766490a6fd3531865095fda6c0eeb6d
 github.com/juju/testing	git	87af103ad72f6ef2b391252bab51d217659e0af3	
 github.com/juju/txn	git	ee0346875f2ae9a21442f3ff64409f750f37afbc	
 github.com/juju/utils	git	27f6e1b91f3ca1da6789e1641a115f284bf49833	
-gopkg.in/juju/charm.v3	git	eb7dae8ed9dfa44b89e4e8eee6e21f80e31a3b69	
+gopkg.in/juju/charm.v3	git	a06606aa4ae97d21e63ba4954b7242198612f076	
 github.com/juju/syslog	git	2b69d6582feb16ff8b6d644495e16c2d8314fcb8	
 gopkg.in/mgo.v2	git	dc255bb679efa273b6544a03261c4053505498a4	
 gopkg.in/natefinch/lumberjack.v2	git	d28785c2f27cd682d872df46ccd8232843629f54	

worker/uniter/context.go

@@ -101,6 +101,9 @@ type HookContext struct {
 
 	// metrics are the metrics recorded by calls to add-metric
 	metrics []jujuc.Metric
+
+	// canAddMetrics specifies whether the hook allows recording metrics
+	canAddMetrics bool
 }
 
 func NewHookContext(
@@ -115,6 +118,7 @@ func NewHookContext(
 	serviceOwner string,
 	proxySettings proxy.Settings,
 	actionParams map[string]interface{},
+	canAddMetrics bool,
 ) (*HookContext, error) {
 	ctx := &HookContext{
 		unit:           unit,
@@ -128,6 +132,7 @@ func NewHookContext(
 		serviceOwner:   serviceOwner,
 		proxySettings:  proxySettings,
 		actionParams:   actionParams,
+		canAddMetrics:  canAddMetrics,
 	}
 	// Get and cache the addresses.
 	var err error
@@ -208,6 +213,9 @@ func (ctx *HookContext) RelationIds() []int {
 
 // AddMetrics adds metrics to the hook context.
 func (ctx *HookContext) AddMetrics(key, value string, created time.Time) error {
+	if !ctx.canAddMetrics {
+		return fmt.Errorf("metrics disabled")
+	}
 	ctx.metrics = append(ctx.metrics, jujuc.Metric{key, value, created})
 	return nil
 }
@@ -322,7 +330,7 @@ func (ctx *HookContext) finalizeContext(process string, err error) error {
 	// TODO (tasdomas) 2014 09 03: context finalization needs to modified to apply all
 	//                             changes in one api call to minimize the risk
 	//                             of partial failures.
-	if len(ctx.metrics) > 0 {
+	if ctx.canAddMetrics && len(ctx.metrics) > 0 {
 		if writeChanges {
 			metrics := make([]params.Metric, len(ctx.metrics))
 			for i, metric := range ctx.metrics {

worker/uniter/context_test.go

@@ -252,7 +252,7 @@ func (s *RunHookSuite) TestRunHook(c *gc.C) {
 	c.Assert(err, gc.IsNil)
 	for i, t := range runHookTests {
 		c.Logf("\ntest %d: %s; perm %v", i, t.summary, t.spec.perm)
-		ctx := s.getHookContext(c, uuid.String(), t.relid, t.remote, t.proxySettings)
+		ctx := s.getHookContext(c, uuid.String(), t.relid, t.remote, t.proxySettings, false)
 		var charmDir, outPath string
 		var hookExists bool
 		if t.spec.perm == 0 {
@@ -304,7 +304,7 @@ func (s *RunHookSuite) TestRunHookRelationFlushing(c *gc.C) {
 	// Create a charm with a breaking hook.
 	uuid, err := utils.NewUUID()
 	c.Assert(err, gc.IsNil)
-	ctx := s.getHookContext(c, uuid.String(), -1, "", noProxies)
+	ctx := s.getHookContext(c, uuid.String(), -1, "", noProxies, false)
 	charmDir, _ := makeCharm(c, hookSpec{
 		name: "something-happened",
 		perm: 0700,
@@ -381,17 +381,17 @@ func (s *RunHookSuite) TestRunHookRelationFlushing(c *gc.C) {
 func (s *RunHookSuite) TestRunHookMetricSending(c *gc.C) {
 	uuid, err := utils.NewUUID()
 	c.Assert(err, gc.IsNil)
-	ctx := s.getHookContext(c, uuid.String(), -1, "", noProxies)
+	ctx := s.getHookContext(c, uuid.String(), -1, "", noProxies, true)
 	charmDir, _ := makeCharm(c, hookSpec{
-		name: "something-happened",
+		name: "collect-metrics",
 		perm: 0700,
 	})
 
 	now := time.Now()
 	ctx.AddMetrics("key", "50", now)
 
 	// Run the hook.
-	err = ctx.RunHook("something-happened", charmDir, c.MkDir(), "/path/to/socket")
+	err = ctx.RunHook("collect-metrics", charmDir, c.MkDir(), "/path/to/socket")
 	c.Assert(err, gc.IsNil)
 
 	metricBatches, err := s.State.MetricBatches()
@@ -403,6 +403,28 @@ func (s *RunHookSuite) TestRunHookMetricSending(c *gc.C) {
 	c.Assert(metrics[0].Value, gc.Equals, "50")
 }
 
+func (s *RunHookSuite) TestRunHookMetricSendingDisabled(c *gc.C) {
+	uuid, err := utils.NewUUID()
+	c.Assert(err, gc.IsNil)
+	ctx := s.getHookContext(c, uuid.String(), -1, "", noProxies, false)
+	charmDir, _ := makeCharm(c, hookSpec{
+		name: "some-hook",
+		perm: 0700,
+	})
+
+	now := time.Now()
+	err = ctx.AddMetrics("key", "50", now)
+	c.Assert(err, gc.ErrorMatches, "metrics disabled")
+
+	// Run the hook.
+	err = ctx.RunHook("some-hook", charmDir, c.MkDir(), "/path/to/socket")
+	c.Assert(err, gc.IsNil)
+
+	metricBatches, err := s.State.MetricBatches()
+	c.Assert(err, gc.IsNil)
+	c.Assert(metricBatches, gc.HasLen, 0)
+}
+
 type ContextRelationSuite struct {
 	testing.JujuConnSuite
 	svc *state.Service
@@ -619,7 +641,7 @@ func (s *InterfaceSuite) GetContext(c *gc.C, relId int,
 	remoteName string) jujuc.Context {
 	uuid, err := utils.NewUUID()
 	c.Assert(err, gc.IsNil)
-	return s.HookContextSuite.getHookContext(c, uuid.String(), relId, remoteName, noProxies)
+	return s.HookContextSuite.getHookContext(c, uuid.String(), relId, remoteName, noProxies, false)
 }
 
 func (s *InterfaceSuite) TestUtils(c *gc.C) {
@@ -771,14 +793,14 @@ func (s *HookContextSuite) AddContextRelation(c *gc.C, name string) {
 }
 
 func (s *HookContextSuite) getHookContext(c *gc.C, uuid string, relid int,
-	remote string, proxies proxy.Settings) *uniter.HookContext {
+	remote string, proxies proxy.Settings, addMetrics bool) *uniter.HookContext {
 	if relid != -1 {
 		_, found := s.relctxs[relid]
 		c.Assert(found, jc.IsTrue)
 	}
 	context, err := uniter.NewHookContext(s.apiUnit, "TestCtx", uuid,
 		"test-env-name", relid, remote, s.relctxs, apiAddrs, "test-owner",
-		proxies, map[string]interface{}(nil))
+		proxies, map[string]interface{}(nil), addMetrics)
 	c.Assert(err, gc.IsNil)
 	return context
 }
@@ -805,14 +827,14 @@ type RunCommandSuite struct {
 
 var _ = gc.Suite(&RunCommandSuite{})
 
-func (s *RunCommandSuite) getHookContext(c *gc.C) *uniter.HookContext {
+func (s *RunCommandSuite) getHookContext(c *gc.C, addMetrics bool) *uniter.HookContext {
 	uuid, err := utils.NewUUID()
 	c.Assert(err, gc.IsNil)
-	return s.HookContextSuite.getHookContext(c, uuid.String(), -1, "", noProxies)
+	return s.HookContextSuite.getHookContext(c, uuid.String(), -1, "", noProxies, addMetrics)
 }
 
 func (s *RunCommandSuite) TestRunCommandsHasEnvironSet(c *gc.C) {
-	context := s.getHookContext(c)
+	context := s.getHookContext(c, false)
 	charmDir := c.MkDir()
 	result, err := context.RunCommands("env | sort", charmDir, "/path/to/tools", "/path/to/socket")
 	c.Assert(err, gc.IsNil)
@@ -839,7 +861,7 @@ func (s *RunCommandSuite) TestRunCommandsHasEnvironSet(c *gc.C) {
 }
 
 func (s *RunCommandSuite) TestRunCommandsStdOutAndErrAndRC(c *gc.C) {
-	context := s.getHookContext(c)
+	context := s.getHookContext(c, false)
 	charmDir := c.MkDir()
 	commands := `
 echo this is standard out

worker/uniter/hook/hook.go

@@ -41,7 +41,7 @@ func (hi Info) Validate() error {
 			return fmt.Errorf("%q hook requires a remote unit", hi.Kind)
 		}
 		fallthrough
-	case hooks.Install, hooks.Start, hooks.ConfigChanged, hooks.UpgradeCharm, hooks.Stop, hooks.RelationBroken:
+	case hooks.Install, hooks.Start, hooks.ConfigChanged, hooks.UpgradeCharm, hooks.Stop, hooks.RelationBroken, hooks.CollectMetrics:
 		return nil
 	case hooks.ActionRequested:
 		if !names.IsValidAction(hi.ActionId) {

worker/uniter/hook/hook_test.go

@@ -40,6 +40,7 @@ var validateTests = []struct {
 	{hook.Info{Kind: hooks.Install}, ""},
 	{hook.Info{Kind: hooks.Start}, ""},
 	{hook.Info{Kind: hooks.ConfigChanged}, ""},
+	{hook.Info{Kind: hooks.CollectMetrics}, ""},
 	{
 		hook.Info{Kind: hooks.ActionRequested},
 		`action id "" cannot be parsed as an action tag`,

worker/uniter/jujuc/add-metric.go

@@ -5,6 +5,7 @@ package jujuc
 
 import (
 	"fmt"
+	"os"
 	"strconv"
 	"strings"
 	"time"
@@ -66,6 +67,7 @@ func (c *AddMetricCommand) Run(ctx *cmd.Context) (err error) {
 	for _, metric := range c.Metrics {
 		err := c.ctx.AddMetrics(metric.Key, metric.Value, metric.Time)
 		if err != nil {
+			fmt.Fprintf(os.Stderr, "can't add metrics!")
 			return errors.Annotate(err, "cannot record metric")
 		}
 	}

worker/uniter/jujuc/add-metric_test.go

@@ -35,67 +35,83 @@ purpose: send metrics
 
 func (s *AddMetricSuite) TestAddMetric(c *gc.C) {
 	testCases := []struct {
-		about  string
-		cmd    []string
-		result int
-		stdout string
-		stderr string
-		expect []jujuc.Metric
+		about         string
+		cmd           []string
+		canAddMetrics bool
+		result        int
+		stdout        string
+		stderr        string
+		expect        []jujuc.Metric
 	}{
 		{
 			"add single metric",
 			[]string{"add-metric", "key=50"},
+			true,
 			0,
 			"",
 			"",
 			[]jujuc.Metric{{"key", "50", time.Now()}},
 		}, {
 			"no parameters error",
 			[]string{"add-metric"},
+			true,
 			2,
 			"",
 			"error: no metrics specified\n",
 			nil,
 		}, {
 			"invalid metric value",
 			[]string{"add-metric", "key=invalidvalue"},
+			true,
 			2,
 			"",
 			"error: invalid value type: expected float, got \"invalidvalue\"\n",
 			nil,
 		}, {
 			"invalid argument format",
 			[]string{"add-metric", "key"},
+			true,
 			2,
 			"",
 			"error: expected \"key=value\", got \"key\"\n",
 			nil,
 		}, {
 			"invalid argument format",
 			[]string{"add-metric", "=key"},
+			true,
 			2,
 			"",
 			"error: expected \"key=value\", got \"=key\"\n",
 			nil,
 		}, {
 			"multiple metrics",
 			[]string{"add-metric", "key=60", "key2=50.4"},
+			true,
 			0,
 			"",
 			"",
 			[]jujuc.Metric{{"key", "60", time.Now()}, {"key2", "50.4", time.Now()}},
 		}, {
 			"multiple metrics, matching keys",
 			[]string{"add-metric", "key=60", "key=50.4"},
+			true,
 			0,
 			"",
 			"",
 			[]jujuc.Metric{{"key", "60", time.Now()}, {"key", "50.4", time.Now()}},
-		},
-	}
+		}, {
+			"can't add metrics",
+			[]string{"add-metric", "key=60", "key2=50.4"},
+			false,
+			1,
+			"",
+			"error: cannot record metric: metrics disabled\n",
+			nil,
+		}}
 	for i, t := range testCases {
 		c.Logf("test %d: %s", i, t.about)
 		hctx := s.GetHookContext(c, -1, "")
+		hctx.canAddMetrics = t.canAddMetrics
 		com, err := jujuc.NewCommand(hctx, t.cmd[0])
 		c.Assert(err, gc.IsNil)
 		ctx := testing.Context(c)

worker/uniter/jujuc/util_test.go

@@ -81,15 +81,19 @@ func setSettings(c *gc.C, ru *state.RelationUnit, settings map[string]interface{
 }
 
 type Context struct {
-	actionParams map[string]interface{}
-	ports        set.Strings
-	relid        int
-	remote       string
-	rels         map[int]*ContextRelation
-	metrics      []jujuc.Metric
+	actionParams  map[string]interface{}
+	ports         set.Strings
+	relid         int
+	remote        string
+	rels          map[int]*ContextRelation
+	metrics       []jujuc.Metric
+	canAddMetrics bool
 }
 
 func (c *Context) AddMetrics(key, value string, created time.Time) error {
+	if !c.canAddMetrics {
+		return fmt.Errorf("metrics disabled")
+	}
 	c.metrics = append(c.metrics, jujuc.Metric{key, value, created})
 	return nil
 }

worker/uniter/uniter.go

@@ -354,7 +354,7 @@ func (u *Uniter) deploy(curl *corecharm.URL, reason Op) error {
 // operation is not affected by the error.
 var errHookFailed = stderrors.New("hook execution failed")
 
-func (u *Uniter) getHookContext(hctxId string, relationId int, remoteUnitName string, actionParams map[string]interface{}) (context *HookContext, err error) {
+func (u *Uniter) getHookContext(hctxId string, hookKind hooks.Kind, relationId int, remoteUnitName string, actionParams map[string]interface{}) (context *HookContext, err error) {
 
 	apiAddrs, err := u.st.APIAddresses()
 	if err != nil {
@@ -372,11 +372,14 @@ func (u *Uniter) getHookContext(hctxId string, relationId int, remoteUnitName st
 	u.proxyMutex.Lock()
 	defer u.proxyMutex.Unlock()
 
+	// Metrics can only be added in collect-metrics hooks.
+	canAddMetrics := hookKind == hooks.CollectMetrics
+
 	// Make a copy of the proxy settings.
 	proxySettings := u.proxy
 	return NewHookContext(u.unit, hctxId, u.uuid, u.envName, relationId,
 		remoteUnitName, ctxRelations, apiAddrs, ownerTag, proxySettings,
-		actionParams)
+		actionParams, canAddMetrics)
 }
 
 func (u *Uniter) acquireHookLock(message string) (err error) {
@@ -426,7 +429,7 @@ func (u *Uniter) RunCommands(commands string) (results *exec.ExecResponse, err e
 	}
 	defer u.hookLock.Unlock()
 
-	hctx, err := u.getHookContext(hctxId, -1, "", map[string]interface{}(nil))
+	hctx, err := u.getHookContext(hctxId, hooks.Kind(""), -1, "", map[string]interface{}(nil))
 	if err != nil {
 		return nil, err
 	}
@@ -525,7 +528,7 @@ func (u *Uniter) runHook(hi hook.Info) (err error) {
 	}
 	defer u.hookLock.Unlock()
 
-	hctx, err := u.getHookContext(hctxId, relationId, hi.RemoteUnit, actionParams)
+	hctx, err := u.getHookContext(hctxId, hi.Kind, relationId, hi.RemoteUnit, actionParams)
 	if err != nil {
 		return err
 	}