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

Provide metric naming and strategy overrides in the prometheus sink #710

Open
binarylogic opened this issue Aug 5, 2019 · 11 comments
Open

Provide metric naming and strategy overrides in the prometheus sink #710

binarylogic opened this issue Aug 5, 2019 · 11 comments

Comments

@binarylogic
Copy link
Member

@binarylogic binarylogic commented Aug 5, 2019

Currently the prometheus sink does not offer the ability to rename or change metric types. This is described in detail in point 3 via #675 (comment). The config interface should look something like:

[sinks.prometheus_id]
  type = "prometheus"
  
  # Renames all foo.* metrics to bazz.*
  [[sinks.prometheus_id.metrics]]
    match_name = "foo.*" # regex pattern
    name = "bazz.$1"
  
  # Changes all histogram type metrics to a summary
  [[sinks.prometheus_id.metrics]]
    match_type = "histogram"
    type = "summary"

  # Changes all bar.* metrics to a histogram with the specified buckets
  [[sinks.prometheus_id.metrics]]
    match_name = "bar.*" # regex pattern
    type = "histogram"
    buckets = [0.1, 0.25, 0.5, 0.75, 1] # optional

  # ... and so on ...

Requirements

  1. The match_name is just a name I came up with, feel free to rename this.
  2. The match_name should support regex, but feel free to provide a limited matching pattern if we have a reason for it (such as a performance benefit).
  3. We should not support every conversion if it adds a ton of complexity (ex: set to counter). In general, we do not need to support unrealistic edge cases if they provide a lot of overhead.
  4. The first match wins and the order they are executed is bottom-up. Meaning the last rule defined will be executed first. Note the proposed defaults here: #711 (comment).
  5. Metric renaming and definitions are likely to live in every metrics sink (ex: aws_cloudwatch_metrics). It might be worth thinking about ways to make this simple, although each metrics sink will have specific settings (ex: buckets) so I'm unsure if this can be fully extracted.
@binarylogic

This comment has been minimized.

Copy link
Member Author

@binarylogic binarylogic commented Aug 18, 2019

@lukesteensen do you mind reviewing this issue to make sure there aren't any glaring issues? If you think this is good can you remove the meta: needs review label?

@lukesteensen

This comment has been minimized.

Copy link
Member

@lukesteensen lukesteensen commented Aug 19, 2019

Makes sense, just a few comments:

  • I'd probably put them under an overrides key rather than a metrics key in the config, which didn't feel terribly clear.

  • I might prefer something like name_pattern over match_name?

  • We'll need to think through how to handle incoming events where the specified type doesn't make sense. Right now I think we implicitly branch based on the type first. If this becomes a pre-processing step, we'll have to decide which conversions make sense to support and what to do with events where there is no conversion. My guess is that there are only a very limited number of conversions that make logical sense, so we definitely shouldn't spend extra time trying to support weird cases.

  • There seem to be two types of these: (1) things like renames (which are simple event -> event transforms), and (2) sink-specific config overrides (e.g. buckets). While it might make sense to expose them to the user in the same way, I'm not sure if they should be implemented in the same way. We could also consider splitting out (1) into a normal transform instead of having a shared implementation within sinks.

@loony-bean

This comment has been minimized.

Copy link
Contributor

@loony-bean loony-bean commented Aug 21, 2019

#377 looks relevant.

@lukesteensen

This comment has been minimized.

Copy link
Member

@lukesteensen lukesteensen commented Aug 21, 2019

I'm not sure about #377. It'd be simple, but almost too simple to be worth its own implementation and docs. Unless there's a strong demand, I wouldn't worry about it as we work on this issue.

@loony-bean

This comment has been minimized.

Copy link
Contributor

@loony-bean loony-bean commented Aug 21, 2019

Yeah, I just thought that it was capturing the idea of

We could also consider splitting out (1) into a normal transform instead of having a shared implementation within sinks.

@loony-bean

This comment has been minimized.

Copy link
Contributor

@loony-bean loony-bean commented Aug 22, 2019

Also it seems that string interpolation could be useful in this context. But I don't see a clear path of doing that, because we do not pass implicit fields to metrics.

@thoughtpolice

This comment has been minimized.

Copy link
Contributor

@thoughtpolice thoughtpolice commented Nov 23, 2019

This would be an excellent transform to have. My current use case is re-exporting statsd metrics to the prometheus sink, which in some cases will actually work very well already if you control the code carefully. But some cases are impossible: many applications use a name like foo.bar in order to register statsd metrics, which are invalid promethus stat names. In such a setup I could do something like this:

[sinks.prometheus_id]
  type = "prometheus"
  [[sinks.prometheus_id.metrics]]
    match_name = "(.?)\.(.?)"
    name = "$1_$2"

which would be excellent!

@loony-bean

This comment has been minimized.

Copy link
Contributor

@loony-bean loony-bean commented Nov 24, 2019

^ @lukesteensen I believe we could add some names normalisation into the Prometheus sink. I don't really think we need a transform or special configuration for that.

Replacing unsupported characters with underscores seems natural, and we were actually doing a similar thing for internal metrics.

@thoughtpolice

This comment has been minimized.

Copy link
Contributor

@thoughtpolice thoughtpolice commented Nov 25, 2019

I'd be willing to write a patch for some basic normalization rules like underscore-to-period if that's fine and easy to do for the Prometheus sink.

@lukesteensen

This comment has been minimized.

Copy link
Member

@lukesteensen lukesteensen commented Nov 25, 2019

Normalizing metric names that are invalid for Prometheus seems like a perfectly reasonable thing for the Prometheus sink to do. That should get quite a bit easier with #1217, since there is only a single name field to worry about. It should be quite straightforward to do early in the start_send implementation. Does that sound right to you @loony-bean?

@loony-bean

This comment has been minimized.

Copy link
Contributor

@loony-bean loony-bean commented Nov 26, 2019

@thoughtpolice Sure!

According to this doc metric names must match [a-zA-Z_:][a-zA-Z0-9_:]* regex, and label names must match this one [a-zA-Z_][a-zA-Z0-9_]*. The rule could be to replace unsupported symbols with '_'.

Please let me know if you'll have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.