Add support for tagged metrics #3147

chelseakomlo · 2017-08-31T16:32:41Z

After the HTTP endpoint for tagged metrics is merged, extensive QA will be useful.

dadgar · 2017-08-31T17:24:13Z

client/config/config.go

@@ -179,6 +179,14 @@ type Config struct {
 	// NoHostUUID disables using the host's UUID and will force generation of a
 	// random UUID.
 	NoHostUUID bool
+
+	// DisableTaggedMetrics determines whether metrics will be displayed via a
+	// key/value/tag format, or simply a key/value format


Will the parsing of the config in the agent be a separate PR?

dadgar · 2017-08-31T17:25:53Z

client/client.go

-	metrics.SetGauge([]string{"client", "host", "memory", nodeID, "available"}, float32(hStats.Memory.Available))
-	metrics.SetGauge([]string{"client", "host", "memory", nodeID, "used"}, float32(hStats.Memory.Used))
-	metrics.SetGauge([]string{"client", "host", "memory", nodeID, "free"}, float32(hStats.Memory.Free))
+	hStats := c.hostStatsCollector.Stats()


Call these in the emitHostStats and pass it in to all the methods?

I was considering creating a separate ClientMetrics object and setting hStats, nodeID, etc as fields. What do you think about that, or should this logic be kept in Client?

Maybe if we find that we add more than just the stats and nodeID but for just those two I don't think it is worth it.

Base labels could also be a field- but agree, we can not do it for now and keep it as an option for later.

dadgar · 2017-08-31T17:28:36Z

client/task_runner.go

-	if ru.ResourceUsage.MemoryStats != nil && r.config.PublishAllocationMetrics {
+func (r *TaskRunner) setGaugeForMemory(ru *cstructs.TaskResourceUsage) {
+	if !r.config.DisableTaggedMetrics {
+		labels := []metrics.Label{{"job", r.alloc.Job.Name}, {"task_group", r.alloc.TaskGroup}, {"alloc_id", r.alloc.ID}, {"task", r.task.Name}}


Can we add a field to the task runner struct that holds these baseLabels and create it once when creating the task runner. Avoid making a bunch of new arrays every time

dadgar · 2017-08-31T17:31:56Z

client/client.go


-	metrics.SetGauge([]string{"uptime"}, float32(hStats.Uptime))
+	if !c.config.DisableTaggedMetrics {
+		labels := []metrics.Label{{"node_id", nodeID}, {"datacenter", c.Node().Datacenter}}


Why don't the others have datacenter?

Create a base labels on the client as well and where addition labels are needed they can just append and create a new slice?

jippi · 2017-08-31T18:14:17Z

having region and dc in all metrics would be the bomb!

dadgar · 2017-08-31T20:52:42Z

client/client.go

-	metrics.SetGauge([]string{"uptime"}, float32(hStats.Uptime))
+func (c *Client) setGaugeForUptime(hStats *stats.HostStats) {
+	if !c.config.DisableTaggedMetrics {
+		labels := []metrics.Label{{"datacenter", c.Node().Datacenter}}


Why not baseLabels?

jippi · 2017-09-01T07:09:34Z

client/client.go

@@ -187,6 +191,8 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic
 		serversDiscoveredCh: make(chan struct{}),
 	}

+	c.baseLabels = []metrics.Label{{"node_id", c.Node().ID}, {"datacenter", c.Node().Datacenter}}


would it be possible to include region too? would make it easier to work with the metrics inside datadog and influx when you have one common metrics storage cross env

Including the region will take a bit more work and is outside the scope for this PR, but we will definitely keep this in mind for the future. Thanks for the suggestion!

dadgar · 2017-09-01T22:33:55Z

client/client.go

@@ -284,6 +288,10 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic
 	// Start collecting stats
 	go c.emitStats()

+	// Assign labels at the latest possible moment so the information expected
+	// is ready
+	c.baseLabels = []metrics.Label{{Name: "node_id", Value: c.Node().ID}, {Name: "datacenter", Value: c.Node().Datacenter}}


Probably want this before you start the emitStats go routine right above it

If anything you could put this in emitStats itself before it starts the loop.

dadgar · 2017-09-01T22:34:47Z

client/client.go

@@ -284,6 +288,10 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic
 	// Start collecting stats
 	go c.emitStats()

+	// Assign labels at the latest possible moment so the information expected
+	// is ready
+	c.baseLabels = []metrics.Label{{Name: "node_id", Value: c.Node().ID}, {Name: "datacenter", Value: c.Node().Datacenter}}


If anything you could put this in emitStats itself before it starts the loop.

dadgar · 2017-09-01T22:35:06Z

client/client.go

-	metrics.SetGauge([]string{"client", "host", "memory", nodeID, "available"}, float32(hStats.Memory.Available))
-	metrics.SetGauge([]string{"client", "host", "memory", nodeID, "used"}, float32(hStats.Memory.Used))
-	metrics.SetGauge([]string{"client", "host", "memory", nodeID, "free"}, float32(hStats.Memory.Free))
+func (c *Client) setGaugeForMemoryStats(nodeID string, hStats *stats.HostStats) {


Please add comments on all the new functions

dadgar · 2017-09-01T22:41:31Z

client/client.go

+	c.setGaugeForUptime(hStats)
+	c.setGaugeForCPUStats(nodeID, hStats)
+	c.setGaugeForDiskStats(nodeID, hStats)
+	c.setGaugeForAllocationStats(nodeID)


This probably shouldn't be guarded by c.config.PublishNodeMetrics. This should be in emitClientMetrics

dadgar · 2017-09-01T22:43:33Z

client/client_test.go

+	}
+
+	nodeID := client.Node().ID
+	for _, e := range client.baseLabels {


Assert length of base labels != 0

dadgar · 2017-09-01T22:45:19Z

client/task_runner.go

+// emitStats emits resource usage stats of tasks to remote metrics collector
+// sinks
+func (r *TaskRunner) emitStats(ru *cstructs.TaskResourceUsage) {
+	if ru.ResourceUsage.MemoryStats != nil && r.config.PublishAllocationMetrics {


func (r *TaskRunner) emitStats(ru *cstructs.TaskResourceUsage) { if !r.config.PublishAllocationMetrics { return } if ru.ResourceUsage.MemoryStats != nil { ... } ....

dadgar · 2017-09-01T22:46:01Z

command/agent/config.go

+			GCInodeUsageThreshold:      70,
+			GCMaxAllocs:                50,
+			NoHostUUID:                 helper.BoolToPtr(true),
+			DisableTaggedMetrics:       false,


Unneeded since the default value for booleans is false.

dadgar · 2017-09-01T22:47:23Z

command/agent/config.go

@@ -1032,6 +1042,14 @@ func (a *ClientConfig) Merge(b *ClientConfig) *ClientConfig {
 		result.NoHostUUID = b.NoHostUUID
 	}

+	if b.DisableTaggedMetrics {


Merge test: https://github.com/hashicorp/nomad/blob/master/command/agent/config_test.go#L71

github-actions · 2023-03-24T02:12:52Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

chelseakomlo changed the title ~~WIP: Add support tagged metrics~~ WIP: Add support for tagged metrics Aug 31, 2017

chelseakomlo force-pushed the b-tagged-metrics branch from 6df1fcc to f350355 Compare August 31, 2017 17:14

dadgar requested changes Aug 31, 2017

View reviewed changes

dadgar reviewed Aug 31, 2017

View reviewed changes

jippi reviewed Sep 1, 2017

View reviewed changes

chelseakomlo changed the title ~~WIP: Add support for tagged metrics~~ Add support for tagged metrics Sep 1, 2017

chelseakomlo force-pushed the b-tagged-metrics branch 2 times, most recently from 79bbe58 to 7bacaff Compare September 1, 2017 20:46

dadgar reviewed Sep 1, 2017

View reviewed changes

dadgar requested changes Sep 1, 2017

View reviewed changes

chelseakomlo added 11 commits September 5, 2017 14:11

vendor latest version of go-metrics and datadog-go/statsd

2c26135

add metrics options to client config

5efb4fb

emit metrics using labels, add option for backwards compatibility

7a96f92

create base labels to be used in every metric

50ab667

pass in commonly used values

a6eeede

refactor to use baseLabels

fce72a1

parse config for metrics fields

a1e81ab

remove prints during test

a265d86

update comments

b6e104f

labels depend on full setup of client beforehand

3c07100

fixups from code review

681a3f3

chelseakomlo force-pushed the b-tagged-metrics branch from 9c32be1 to 681a3f3 Compare September 5, 2017 14:16

fix up travis test failure via race condition

ef4aef0

chelseakomlo force-pushed the b-tagged-metrics branch from f337712 to ef4aef0 Compare September 5, 2017 15:05

chelseakomlo mentioned this pull request Sep 5, 2017

Tagged metrics API #3162

Merged

chelseakomlo force-pushed the b-tagged-metrics branch from 8f53f39 to 4a03f8f Compare September 5, 2017 18:19

final code review fixups

68686cd

chelseakomlo force-pushed the b-tagged-metrics branch from 4a03f8f to 68686cd Compare September 5, 2017 18:48

add changelog

0d434d0

dadgar approved these changes Sep 5, 2017

View reviewed changes

chelseakomlo merged commit 949df54 into master Sep 5, 2017

chelseakomlo deleted the b-tagged-metrics branch September 5, 2017 19:55

chelseakomlo mentioned this pull request Sep 11, 2017

Fix panic in emitting tagged allocation metrics #3191

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tagged metrics #3147

Add support for tagged metrics #3147

chelseakomlo commented Aug 31, 2017 •

edited

Loading

dadgar Aug 31, 2017

dadgar Aug 31, 2017

chelseakomlo Aug 31, 2017

dadgar Aug 31, 2017

chelseakomlo Aug 31, 2017

dadgar Aug 31, 2017

dadgar Aug 31, 2017

dadgar Aug 31, 2017

jippi commented Aug 31, 2017

dadgar Aug 31, 2017

jippi Sep 1, 2017

chelseakomlo Sep 1, 2017

dadgar Sep 1, 2017

dadgar Sep 1, 2017

dadgar Sep 1, 2017

dadgar Sep 1, 2017

dadgar Sep 1, 2017 •

edited

Loading

dadgar Sep 1, 2017

dadgar Sep 1, 2017

dadgar Sep 1, 2017

dadgar Sep 1, 2017

github-actions bot commented Mar 24, 2023

Add support for tagged metrics #3147

Add support for tagged metrics #3147

Conversation

chelseakomlo commented Aug 31, 2017 • edited Loading

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

jippi commented Aug 31, 2017

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

dadgar Sep 1, 2017 • edited Loading

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

github-actions bot commented Mar 24, 2023

chelseakomlo commented Aug 31, 2017 •

edited

Loading

dadgar Sep 1, 2017 •

edited

Loading