dropwizard / metrics

:chart_with_upwards_trend: Capturing JVM- and application-level metrics. So you know what's going on.
https://metrics.dropwizard.io
Apache License 2.0
7.83k stars 1.81k forks source link

MetricName well-defined toString + valueOf(String) #2167

Open mbrannstrom opened 2 years ago

mbrannstrom commented 2 years ago

To simplify migration from the plain dotted.name syntax (in 4.x versions), usually represented as a Java String, the new tagged MetricName should have a well-defined string syntax.

For example, dropwizard-metrics-graphite uses a String for its' prefix in the configuration.

Currently the MetricName have the toString() representation of dotted.name{tag1=value, tag2=value}, i.e. dottedName + Map.toString(). However, without escaping that representation is not parsable.

As a comparison:

Discussion:

Should we stick to the existing string representation, or use e.g. the InfluxDB syntax, which is well-defined (explicit escaping rules), slimmer than Prometheus' and more liberate in allowed characters? The InfluxDB line protocol syntax is also backwards compatible when not using tags, e.g. dotted.name vs dotted.name,with=tags

Update:

Note: A common "prefix" is to add tags for an application, e.g. environment=production,region=eu-west without the "dotted.name" part.

What do you think? Should we go for a InfluxDB-inspired syntax?

baharclerode commented 2 years ago

the new tagged MetricName should have a well-defined string syntax.

With the addition of structured tagging, it should be configurable, not well defined, since it needs to match and work with the tags people already have embedded in their 4.x metric names. I might have 3 different reporters configured, and each one might need tags to be surfaced in a different format. More specific to my situation, for a jump to 5.X to be reasonable, I would need the JSON serialization format to match the tag format I have currently embedded in the 4.X names, because it's not otherwise feasible to upgrade several thousand applications and our metrics processing pipeline to use a different format all at once.

However, without escaping that representation is not parsable.

I'm not sure it's reasonable to expect the MetricName.toString() output to be machine-parsable anymore. The string serialization format of MetricName is dependent upon the metrics reporting system, and no longer an intrinsic property of the name itself; Any single serialization format is going to cause problems for someone. toString() should use a format that is human-parsable, best suited for developers that see it appear in logs or debuggers. dottedName + Map.toString() is actually very good for that, since Map.toString() something that developers are already used to parsing visually.

Perhaps this would be better solved with an interface that reporters can leverage:

public interface MetricNameFormat {
    static MetricNameFormat INFLUXDB = new InfluxDBNameFormat();
    static MetricNameFormat PROMETHUS = new PromethusNameFormat();

    String serialize(MetricName metricName);
}
github-actions[bot] commented 5 months ago

This issue is stale because it has been open 180 days with no activity. Remove the "stale" label or comment or this will be closed in 14 days.

mbrannstrom commented 3 months ago

@baharclerode: I disagree, and you're missing the point here.

Any API should have a well defined behaviour. Not a random behaviour that "just happened" by some arbitrary implementation. In the JDK it is common that toString can be parsed by the corresponding valueOf/parseXxx method. E.g. Integer.toStringvs Integer.valueOf and File.toString = File.getPath vs new File.

The InfluxDB line protocol syntax is an example of how to define the MetricName.toString, that happens to be well-defined and simple (and human readable). It is not intended to be a replacement for serialization for a specific metric database.

Perhaps this would be better solved with an interface that reporters can leverage: public interface MetricNameFormat

This is not a good idea. This serialization should be up to the reporter to e.g. InlfuxDB and Prometheus. Not part of the core Metrics API. The core API should be small, and not affected by new reporters.