Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow multiple dashes in StatsD metric names #381

Merged
merged 1 commit into from
Aug 31, 2021

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
mapping.yml Outdated Show resolved Hide resolved
@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