Allow histograms for timer metrics #66

bakins · 2017-03-13T10:37:12Z

Based on comments in #19 this allows the use of a YAML config file that allows setting histograms for timer metrics per mapping or as the default timer type. Previous behavior is the same when using the "old" config file format.

This is fairly rough and could probably be cleaner. I did the minimum possible to enable histograms as an experiment. Consider this enough to start a conversation.

grobie

LGTM, but @juliusv should take a look as maintainer.

bakins · 2017-06-21T18:42:59Z

Anything I can do to help on this? We've been running my fork for a while and it seems to be working fine.

gerricom · 2017-06-28T11:52:34Z

@bakins: Could you provide a sample configuration file? I do not understand how to map the labels with your yaml-solution... 😕

bakins · 2017-07-10T22:58:41Z

@gerricom sorry for the delay.

This PR was to start the discussion. I can add documentation if people think this is useful.

Example:

"normal" config:

test.dispatcher.*.*.*
name="dispatcher_events_total"
processor="$1"
action="$2"
outcome="$3"
job="test_dispatcher"

YAML version of this:

mappings:
- match: test.dispatcher.*.*.*
  labels:
    name: "dispatcher_events_total"
    processor: "$1"
    action: "$2"
    outcome: "$3"
    job: "test_dispatcher"

To enable histograms for a single mapping:

mappings:
- match: test.dispatcher.*.*.*
  timer_type: histogram
  labels:
    name: "dispatcher_events_total"
...

To enable histograms by default:

defaults: 
  timer_type: histogram

gerricom · 2017-07-17T10:59:23Z

@bakins No problem! Thanks for the example. I must have simply missed the match parameter in the YAML configuration.

So after playing around with it: This looks quite useful and I'd like to see this merged. Changing over to the YAML configuration style is very easily done and straightforward. Histograms seem to work as expected.

juliusv · 2017-07-17T13:40:51Z

@bakins Sorry for ignoring this for so long. I think in general changing this to YML is a great idea, to get rid of yet another custom language. Taking a closer look now.

juliusv

Looks pretty good in general. I left some comments. Documentation would be great, as you said.

I think sometime in the future we could maybe even drop the custom config format.

juliusv · 2017-07-17T13:52:05Z

mapper.go


 	mappingsCount.Set(float64(len(parsedMappings)))

 	return nil
 }

+func (m *metricMapper) initFromYamlString(fileContents string) error {
+


style: avoid empty lines at the beginning of a function.

juliusv · 2017-07-17T14:00:39Z

mapper.go


 	mappingsCount.Set(float64(len(parsedMappings)))

 	return nil
 }

+func (m *metricMapper) initFromYamlString(fileContents string) error {


style: Yaml -> YAML (https://github.com/golang/go/wiki/CodeReviewComments#initialisms)

juliusv · 2017-07-17T14:03:00Z

mapper_test.go

@@ -148,7 +148,7 @@ func TestMetricMapper(t *testing.T) {
 				test.bar
 				name="name_bar"
 				label="foo"
-				
+
 				test.foo
 				name="name_foo"
 				label="bar"`,


Would be great to have some YAML-based tests here of course.

juliusv · 2017-07-17T14:06:20Z

exporter.go

+					timerType = b.mapper.Defaults.TimerType
+				}
+
+				if timerType == "histogram" {


As far as I can see, the timer type string isn't validated at config load time, but probably should be?

And then I'd change this to a switch timerType {...} here with a default case that panics. Could be good to extract the strings into named constants too.

juliusv · 2017-07-17T14:10:48Z

exporter.go

+
+				if timerType == "histogram" {
+					histogram := b.Histograms.Get(
+						b.suffix(metricName, "timer"),


Hmm, I think we should only suffix metrics that don't have an explicitly mapped metric name (if there is a mapping, use exactly the metric name provided in the mapping). But I see we also do that for gauges and counters already. So that should probably be fixed in a separate change.

bakins · 2017-07-26T18:52:41Z

This now validates the timer type when it reads the configuration. Also addresses a couple of the style issues. Let me know what you think. I'll add docs to this PR after we think the code looks good.

juliusv · 2017-07-26T18:59:34Z

mapper.go

+func (m *metricMapper) initFromYAMLString(fileContents string) error {
+	var n metricMapper
+
+	fmt.Println(fileContents)


Remove debug Println here and Printf below.

juliusv · 2017-07-26T18:59:59Z

exporter.go

-					log.Errorf(regErrF, metricName, err)
-					conflictingEventStats.WithLabelValues("timer").Inc()
+				switch t {
+				case "histogram":


"histogram" -> timerTypeHistogram?

grobie · 2017-07-27T07:32:46Z

Could you add some tests for the new feature?

bakins · 2017-07-31T21:23:01Z

Added basic config tests.

drawks · 2017-07-31T21:53:57Z

This looks like a very nice change. One critique of mine would be to separate the conversion to yaml config and the addition of histograms as two separate PRs. I'm a big fan of both changes though personally.

juliusv

Couple of last nits and then let's ship this :)

juliusv · 2017-07-31T23:34:11Z

mapper.go

+			return fmt.Errorf("invalid match: %s", currentMapping.Match)
+		}
+
+		// check that label is correct


nit: start all comments upper-cased, end with period.

juliusv · 2017-07-31T23:37:47Z

mapper.go

+		// check that label is correct
+		for k, v := range currentMapping.Labels {
+			label := fmt.Sprintf("%s=%q", k, v)
+			if len(labelLineRE.FindStringSubmatch(label)) != 3 {


It is a bit complicated to first make a k=v string out of this and then match the entire string against labelLineRE. The label value can be anything, so it would be better to just check whether the label name (k) matches identifierRE.

Not sure why I did it this way. fixing.

juliusv · 2017-07-31T23:38:33Z

mapper.go

+		}
+
+		if _, ok := currentMapping.Labels["name"]; !ok {
+			return fmt.Errorf("Line %d: metric mapping didn't set a metric name", i)


"Line" -> "line" (start error strings lower-cased)

FWIW, errors in updateMapping in this same file use uppercase "L". I'll change my to lower.

bakins · 2017-08-01T11:10:30Z

@juliusv Addressed your comments.

@drawks I can split this up if that's the consensus. YAML change would need to be merged first, IMO. In #19 it seems people did not want to mangle the current config format any more. I guess splitting or not is up to @juliusv ?

bakins · 2017-08-01T11:27:10Z

Added initial docs.

juliusv · 2017-08-03T15:25:28Z

@bakins I think it's fine to just merge it as it is this time (YAML + histograms combined), although in the future I would recommend separating changes into multiple PRs.

Thanks for all the patience and sorry for being so slow here! I'll merge it and then we can still do followup cleanups as needed.

bakins · 2017-08-03T18:17:19Z

@juliusv thanks. As with most things, this PR started very simple...

bakins and others added 4 commits March 12, 2017 19:46

Add yaml dependency

df926ae

Add YAML config file and ability to set histograms

3048412

Allow overriding default timing type

1488f0f

getMapping now returns 3 values

641dc37

grobie reviewed Mar 18, 2017

View reviewed changes

SuperQ requested a review from juliusv May 12, 2017 12:51

juliusv reviewed Jul 17, 2017

View reviewed changes

Brian Akins added 3 commits July 26, 2017 14:03

merge with upstream master

74975b8

Style: Yaml -> YAML

e8997f7

Validate timer type while reading config

b467a53

juliusv reviewed Jul 26, 2017

View reviewed changes

cleanup debug statements

2643f15

Brian Akins added 2 commits July 27, 2017 16:58

add simple YAML config tests

c9510f7

add test case for timer types

93243f6

juliusv reviewed Jul 31, 2017

View reviewed changes

Brian Akins added 2 commits August 1, 2017 07:01

Style fixes

c339cc6

simplify metric label key validation

9c1cde6

Simple YAML configuration docs

9bca336

This was referenced Aug 1, 2017

initFromString should allow comments in config strings #67

Closed

Allow for setting "#help" text in mapping config #78

Closed

juliusv merged commit aeab290 into prometheus:master Aug 3, 2017

gerricom mentioned this pull request Aug 4, 2017

New histogram feature with conversion error #88

Closed

drawks mentioned this pull request Aug 8, 2017

metric name should be split from labels in yaml mapping format #90

Closed

bakins deleted the extended-config branch September 19, 2017 20:39

drawks mentioned this pull request Oct 6, 2017

Usage of histogram for timer metrics #19

Closed

bakins mentioned this pull request Nov 14, 2017

Preparation for v0.5.0 release #108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow histograms for timer metrics #66

Allow histograms for timer metrics #66

bakins commented Mar 13, 2017

grobie left a comment

bakins commented Jun 21, 2017

gerricom commented Jun 28, 2017

bakins commented Jul 10, 2017

gerricom commented Jul 17, 2017

juliusv commented Jul 17, 2017

juliusv left a comment

juliusv Jul 17, 2017

juliusv Jul 17, 2017

juliusv Jul 17, 2017

juliusv Jul 17, 2017

juliusv Jul 17, 2017

bakins commented Jul 26, 2017

juliusv Jul 26, 2017

juliusv Jul 26, 2017

grobie commented Jul 27, 2017

bakins commented Jul 31, 2017

drawks commented Jul 31, 2017

juliusv left a comment

juliusv Jul 31, 2017

juliusv Jul 31, 2017

bakins Aug 1, 2017

juliusv Jul 31, 2017

bakins Aug 1, 2017

bakins commented Aug 1, 2017

bakins commented Aug 1, 2017

juliusv commented Aug 3, 2017

bakins commented Aug 3, 2017

Allow histograms for timer metrics #66

Allow histograms for timer metrics #66

Conversation

bakins commented Mar 13, 2017

grobie left a comment

Choose a reason for hiding this comment

bakins commented Jun 21, 2017

gerricom commented Jun 28, 2017

bakins commented Jul 10, 2017

gerricom commented Jul 17, 2017

juliusv commented Jul 17, 2017

juliusv left a 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

bakins commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie commented Jul 27, 2017

bakins commented Jul 31, 2017

drawks commented Jul 31, 2017

juliusv left a 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

bakins commented Aug 1, 2017

bakins commented Aug 1, 2017

juliusv commented Aug 3, 2017

bakins commented Aug 3, 2017