Skip to content

Conversation

@evandam
Copy link
Contributor

@evandam evandam commented Jun 17, 2021

See #380

@matthiasr I don't do any Go development so not sure if there's anything else to consider here 😅

Thanks!

@matthiasr
Copy link
Contributor

There is a test that specifically said "double dashes are illegal" – it is intended to exercise a case that doesn't match this regex. Could you change it to use some other illegal character?

@matthiasr
Copy link
Contributor

Also, please add the DCO sign-off, instructions are behind the Details link

@SuperQ
Copy link
Member

SuperQ commented Jun 17, 2021

It might also be useful to add a positive test like this: (line 1467)

    {
      testName: "Config with multiple dashes",
      config: `mappings:
- match: "*.fabio--requests.count"
  name: "fabio_requests_count"
  help: "Fabio request count"
  labels:
    hostname: "$1"`,
      mappings: mappings{
        {
          statsdMetric: "test.fabio--requests.count",
          name:         "fabio_requests_count",
          labels: map[string]string{
            "hostname": "test",
          },
        },
      },
    },              },

@evandam evandam force-pushed the allow-multi-dash branch 3 times, most recently from cb2f338 to e18c638 Compare June 17, 2021 16:41
@evandam evandam force-pushed the allow-multi-dash branch from e18c638 to abb7ec0 Compare June 17, 2021 16:43
@matthiasr
Copy link
Contributor

Something I am wondering: what happens if we accept a statsd metric with -- but have no mapping for it? Will it be transformed to a valid Prometheus metric?

@evandam
Copy link
Contributor Author

evandam commented Jun 17, 2021

In my super quick testing it seems like it gets dropped if there's no mapping:

level=debug ts=2021-06-17T16:51:00.278Z caller=line.go:214 msg="Bad line from StatsD" line=X
level=debug ts=2021-06-17T16:51:00.290Z caller=listener.go:73 msg="Incoming line" proto=udp line=--.count:0|c
level=debug ts=2021-06-17T16:51:00.290Z caller=listener.go:73 msg="Incoming line" proto=udp line=

A mapping for match: "--" is invalid.

level=error ts=2021-06-17T16:52:24.339Z caller=main.go:340 msg="error loading config" error="invalid match: --"

It seems reasonable enough to me but it's up to you folks!

@matthiasr
Copy link
Contributor

Hmm, but for metrics with a single dash there is an automatic mapping. It will be very confusing to users if some metrics are mapped automatically but others are not. Do you feel up to extending the metric name escaping so that it handles this input?

@evandam
Copy link
Contributor Author

evandam commented Jun 23, 2021

Hey @matthiasr,it sounds like you folks know better than me to get this over the line. I don't write Go either so maybe I'm not the best to handle this if there's more to it than is already done 😅

@matthiasr
Copy link
Contributor

I created a new issue for handling the default case: #389.

Copy link
Contributor

@matthiasr matthiasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, and sorry for holding it up for so long!

@matthiasr matthiasr merged commit a9c883a into prometheus:master Aug 31, 2021
matthiasr added a commit that referenced this pull request Aug 31, 2021
Signed-off-by: Matthias Rampke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants