Reduce number and increase consistency of metric/log labels. #136

fluffle · 2017-10-31T19:46:30Z

Stackdriver (SD) imposes a limit of 10 labels per custom metric. Simply copying event metadata into SD labels results in some metrics hitting that limit.

To address this, drop eventType and the org, space, and application UUIDs from metric labels completely. Create an "applicationPath" label that concatenates together the org, space and application name.

Keep the VM and application index labels separate so that higher level aggregations of the same metric data can be created.

Distinguish between the labels applied to log messages and the labels applied to metrics, because distinguishing logs by origin and event type is useful and there doesn't appear to be a similar label limit for SD logging.

This change is

johnsonj · 2017-10-31T20:22:57Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

src/stackdriver-nozzle/nozzle/label_maker.go, line 41 at r1 (raw file):

}

type labelMap map[string]string

smooth! i like it

src/stackdriver-nozzle/nozzle/label_maker.go, line 43 at r1 (raw file):

type labelMap map[string]string

func (labels labelMap) setIfNotEmpty(key, value string) {

is a pointer receiver not necessary because map is always a reference? (or a value to a reference)

src/stackdriver-nozzle/nozzle/label_maker.go, line 70 at r1 (raw file):

// Build extracts metric metadata from the event envelope and event contained
// within, and constructs a set of StackDriver (SD) metric labels from them.

nit: Stackdriver

src/stackdriver-nozzle/nozzle/label_maker.go, line 78 at r1 (raw file):

// We maintain "deployment" as a separate label to facilitate monitoring
// multple PCF instances within a GCP project, though this does require
// users to name their PCF deployment on bosh something other than "cf".:

Just a note- I'm not sure users have this ability with Ops Manager (today). Though it should be easy with OSS Cloud Foundry.

src/stackdriver-nozzle/nozzle/label_maker.go, line 102 at r1 (raw file):

//     /diego_brain/tps_listener
func (lm *labelMaker) getOriginPath(envelope *events.Envelope) string {
	labels := labelMap{}

what about splitting labelMap to labelMap (setIfNotEmpty), pathMap(setValueOrUnknown, path)? The two functionalities seem distinct

Another thing to consider- path does not need to be map, it could be just a byte.Buffer with something like `appendPathOrUnknown("job", envelope...)

src/stackdriver-nozzle/nozzle/label_maker_test.go, line 145 at r1 (raw file):

				})

				It("adds fields for a resolved app", func() {

Nice addition

Comments from Reviewable

johnsonj · 2017-10-31T21:25:00Z

Big fan of this design btw. The naming/path stuff is good.

fluffle · 2017-11-02T11:25:07Z

I've had to use similar tricks in the past to coalesce overly-large varz maps in borgmon.

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

src/stackdriver-nozzle/nozzle/label_maker.go, line 41 at r1 (raw file):

Previously, johnsonj (Jeff Johnson) wrote…

smooth! i like it

Thanks :-)

I'm a fan of making little helper types like this, it keeps things cleaner IMO.

src/stackdriver-nozzle/nozzle/label_maker.go, line 43 at r1 (raw file):

Previously, johnsonj (Jeff Johnson) wrote…

is a pointer receiver not necessary because map is always a reference? (or a value to a reference)

That's correct, maps are a reference types. I'd only need a pointer receiver to replace the entire map with a new one.

src/stackdriver-nozzle/nozzle/label_maker.go, line 70 at r1 (raw file):

Previously, johnsonj (Jeff Johnson) wrote…

nit: Stackdriver

Done.

src/stackdriver-nozzle/nozzle/label_maker.go, line 78 at r1 (raw file):

Previously, johnsonj (Jeff Johnson) wrote…

Just a note- I'm not sure users have this ability with Ops Manager (today). Though it should be easy with OSS Cloud Foundry.

Thanks for that, I'll start a conversation with the Pivotal folks around how they see differentiating multiple instances working.

src/stackdriver-nozzle/nozzle/label_maker.go, line 102 at r1 (raw file):

Previously, johnsonj (Jeff Johnson) wrote…

what about splitting labelMap to labelMap (setIfNotEmpty), pathMap(setValueOrUnknown, path)? The two functionalities seem distinct

Another thing to consider- path does not need to be map, it could be just a byte.Buffer with something like `appendPathOrUnknown("job", envelope...)

Great points! I got to the current implementation after a few iterations, but never went back to reconsider the use of a map for path construction. A buffer-based implementation would definitely be superior. PTAL?

I'm not sure whether the (much-reduced) labelMap type is really worthwhile, now. I mean, it saves some repetitive lines, but I'm not sure I should bother doing so. WDYT?

src/stackdriver-nozzle/nozzle/label_maker_test.go, line 145 at r1 (raw file):

Previously, johnsonj (Jeff Johnson) wrote…

Nice addition

It's pretty much just copypasta of other tests, but thanks ;-)

Comments from Reviewable

fluffle · 2017-11-02T11:34:38Z

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

src/stackdriver-nozzle/nozzle/label_maker.go, line 43 at r1 (raw file):

Previously, fluffle (Alex Bee) wrote…

That's correct, maps are a reference types. I'd only need a pointer receiver to replace the entire map with a new one.

Ok, so according to Dave Cheney (and he ought to know) there are no reference types in Go, but a map value is really a pointer already:

https://dave.cheney.net/2017/04/29/there-is-no-pass-by-reference-in-go
https://dave.cheney.net/2017/04/30/if-a-map-isnt-a-reference-variable-what-is-it

#TIL

Comments from Reviewable

fluffle · 2017-11-02T16:41:22Z

We have found that e.g. the datadog nozzle prepends the origin to the metric name, so I am going to change this to do the same thing and drop the "origin path" completely. I think the application path concatenation still makes a certain amount of sense, though, because I cannot see a reason why you would want to aggregate stats together for all applications in a space when they most likely do completely different things.

Please don't merge this yet :-)

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

johnsonj · 2017-11-02T17:28:48Z

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

src/stackdriver-nozzle/nozzle/label_maker.go, line 43 at r1 (raw file):

Previously, fluffle (Alex Bee) wrote…

Ok, so according to Dave Cheney (and he ought to know) there are no reference types in Go, but a map value is really a pointer already:

https://dave.cheney.net/2017/04/29/there-is-no-pass-by-reference-in-go
https://dave.cheney.net/2017/04/30/if-a-map-isnt-a-reference-variable-what-is-it

#TIL

Gosh.. This is the one Go thing that I don't quite like but I don't know what would be better.. Thanks for the articles. Interesting case.

src/stackdriver-nozzle/nozzle/label_maker.go, line 102 at r1 (raw file):

Previously, fluffle (Alex Bee) wrote…

Great points! I got to the current implementation after a few iterations, but never went back to reconsider the use of a map for path construction. A buffer-based implementation would definitely be superior. PTAL?

I'm not sure whether the (much-reduced) labelMap type is really worthwhile, now. I mean, it saves some repetitive lines, but I'm not sure I should bother doing so. WDYT?

I think you hit a good balance with this

src/stackdriver-nozzle/nozzle/label_maker.go, line 64 at r3 (raw file):

func (pm *pathMaker) String() string {
	return pm.buf.String()

if you wanted to get really fancy you could embed the buffer:

type pathMaker struct {
   bytes.Buffer
}

Then all the methods of Buffer are available:

path := &pathMather{}
path.addElement(..)

path.String()

Comments from Reviewable

johnsonj · 2017-11-02T17:29:01Z

Reviewed 1 of 2 files at r1, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

fluffle · 2017-11-03T12:55:49Z

Urgh. This got a bit more complex (of course), because I noticed that it would be useful for the log sink to still have the eventType and origin labels, so I had to separate out the two types of labels.

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

src/stackdriver-nozzle/nozzle/label_maker.go, line 64 at r3 (raw file):

Previously, johnsonj (Jeff Johnson) wrote…

if you wanted to get really fancy you could embed the buffer:
type pathMaker struct {
   bytes.Buffer
}
Then all the methods of Buffer are available:
path := &pathMather{}
path.addElement(..)

path.String()

I did consider doing this (cos I agree it's much smoother syntactically) but I got dinged on an internal readability review for similar things recently.

The argument the readability reviewer made was that embedding the type makes the surrounding struct implicitly fulfil all the interfaces the embedded type does. Personally I don't think that's a problem, but apparently it's bad form to do it unless you explicitly want that behaviour. I suspect it would probably be fine here because pathMaker is not an exported type, but meh.

Comments from Reviewable

johnsonj · 2017-11-03T20:32:41Z

LGTM - is this ready from your perspective?

Reviewed 6 of 6 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/stackdriver-nozzle/nozzle/label_maker.go, line 64 at r3 (raw file):

Previously, fluffle (Alex Bee) wrote…

I did consider doing this (cos I agree it's much smoother syntactically) but I got dinged on an internal readability review for similar things recently.

The argument the readability reviewer made was that embedding the type makes the surrounding struct implicitly fulfil all the interfaces the embedded type does. Personally I don't think that's a problem, but apparently it's bad form to do it unless you explicitly want that behaviour. I suspect it would probably be fine here because pathMaker is not an exported type, but meh.

I had that same thought and agree since it's not exported and you're in the package you always have direct access to that embedded object. Also meh. It's kind of in that category of "wow that's cool" but "wow that's unnecessary"

Comments from Reviewable

fluffle · 2017-11-06T09:31:43Z

Yes, I think this is good to go. We have a follow-up planned that will make it possible to set static labels via the configuration. Since there's only going to be one set of nozzles per pcf instance this nicely solves the "multiple instances in one project" problem.

Stackdriver (SD) imposes a limit of 10 labels per custom metric. Simply copying event metadata into SD labels results in some metrics hitting that limit. To address this, drop eventType and the org, space, and application UUIDs from metric labels completely. Create an "applicationPath" label that concatenates together the org, space and application name. Keep the VM and application index labels separate so that higher level aggregations of the same metric data can be created. Distinguish between the labels applied to log messages and the labels applied to metrics, because distinguishing logs by origin and event type is useful and there doesn't appear to be a similar label limit for SD logging.

johnsonj · 2017-11-06T16:26:28Z

LGTM

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

This isn't a metric, it's a label as repoted by cloudfoundry-community#100. We now properly emit it as a label as of cloudfoundry-community#136. This change is simply cleanup.

johnsonj mentioned this pull request Oct 31, 2017

Instance index should be a label not a metric #100

Closed

fluffle force-pushed the pr-prune-metric-labels branch from 6c47cbc to f084461 Compare November 2, 2017 11:24

fluffle force-pushed the pr-prune-metric-labels branch from f084461 to a71a8d9 Compare November 2, 2017 11:28

fluffle force-pushed the pr-prune-metric-labels branch from a71a8d9 to 875d3af Compare November 2, 2017 12:38

fluffle force-pushed the pr-prune-metric-labels branch from 875d3af to 64b917c Compare November 3, 2017 12:44

fluffle changed the title ~~Concatenate event metadata into "paths".~~ Reduce number and increase consistency of metric/log labels. Nov 3, 2017

fluffle force-pushed the pr-prune-metric-labels branch from 64b917c to 1c8e40b Compare November 6, 2017 10:33

fluffle mentioned this pull request Nov 6, 2017

Config options for metric prefix and director name. #144

Merged

johnsonj merged commit 939a9c6 into cloudfoundry-community:develop Nov 6, 2017

This was referenced Nov 7, 2017

nozzle/metric_sink: remove instanceIndex metric #145

Merged

Opt-in to Alpha Labels/Metrics #148

Closed

fluffle deleted the pr-prune-metric-labels branch January 12, 2018 09:57

johnsonj mentioned this pull request Feb 20, 2018

Promote Develop #203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce number and increase consistency of metric/log labels. #136

Reduce number and increase consistency of metric/log labels. #136

fluffle commented Oct 31, 2017 •

edited

Loading

johnsonj commented Oct 31, 2017

johnsonj commented Oct 31, 2017

fluffle commented Nov 2, 2017

fluffle commented Nov 2, 2017

fluffle commented Nov 2, 2017

johnsonj commented Nov 2, 2017

johnsonj commented Nov 2, 2017

fluffle commented Nov 3, 2017

johnsonj commented Nov 3, 2017

fluffle commented Nov 6, 2017

johnsonj commented Nov 6, 2017

Reduce number and increase consistency of metric/log labels. #136

Reduce number and increase consistency of metric/log labels. #136

Conversation

fluffle commented Oct 31, 2017 • edited Loading

johnsonj commented Oct 31, 2017

johnsonj commented Oct 31, 2017

fluffle commented Nov 2, 2017

fluffle commented Nov 2, 2017

fluffle commented Nov 2, 2017

johnsonj commented Nov 2, 2017

johnsonj commented Nov 2, 2017

fluffle commented Nov 3, 2017

johnsonj commented Nov 3, 2017

fluffle commented Nov 6, 2017

johnsonj commented Nov 6, 2017

fluffle commented Oct 31, 2017 •

edited

Loading