Validate naming

[PericlesTheo]

👋 everyone,

Related to #292 I've opened an MR that validates the naming expectations of Prometheus before sending the event.

I'm returning false for now but I am open to ideas as to the best way to handle this.

[benoittgt]

Should we raise instead? Otherwise the error would be pretty silent.

Also I am wondering if could be done somewhere else to avoid extra check on every send_json. For example when the metric is setup?

[PericlesTheo]

My initial thought was to add it in RemoteMetric as such

class RemoteMetric
  def valid?
     # ...
   end
end

But that doesn't cover the custom labels passed to the instance methods of RemoteMetric nor the global custom_labels.

Where were you thinking it would be a nice place?

About the error, I'm not sure 🤔 My initial thoughts were:

• Let's say you are using something like Sentry. You can flood your account with error events quite easily.
• Is it better to have an app crashing in the middle because of metrics?

But I'm up for discussing this.

[PericlesTheo]

And lastly, one can call send_json directly I think so the MR was safeguarding against that as well.

[benoittgt]

You are correct. I think your change is the way to go.

[SamSaffron]

this is a massive behavior change that is breaking, maybe we make this optional default off?

[PericlesTheo]

Hey @SamSaffron, how/where do you suggest we do this?

Would an optional argument in lib/prometheus_exporter/client.rb work?

module PrometheusExporter
  class Client
    def initialize(..., validate_metric_naming: false)
    end
  end
end

If we go with an approach like that, should we raise an error when the naming is invalid instead of returning false as the PR currently suggests since it's now an explicit decision from the developer?

An alternative, that might be an okay middle-ground, is to have a custom metric such as "invalid_naming" or something similar, with the label value of the failed naming. This can remove the decision making from the gem on how to proceed in the cases of errors. Developers can then have an alert on the "invalid_naming" metric and act upon.

Personally, it also satisfies the two concerns I raised here about raising an error (also returning false has the inverse concerns)

About the error, I'm not sure 🤔 My initial thoughts were:

• Let's say you are using something like Sentry. You can flood your account with error events quite easily.
• Is it better to have an app crashing in the middle because of metrics?

The first one is obvious as no errors will be raised. As for the second, I doubt the cardinality of the label value (invalid name) is going to be an issue.

[PericlesTheo]

Hey @SamSaffron any chance you had the time to look into my last message?

[SamSaffron]

I worry about breaking changes, I guess validate_metric_naming: true could be documented and use "metric_name_invalid" or something like that on invalid metrics.