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

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

Open
mbrannstrom opened this issue Sep 20, 2021 · 3 comments
Open

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

mbrannstrom opened this issue Sep 20, 2021 · 3 comments

Comments

@mbrannstrom
Copy link

mbrannstrom commented Sep 20, 2021

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:

  • InfluxDB use the syntax (line protocol syntax): weather,location=us-midwest
  • Prometheus use the syntax bicycle_distance_meters_total{brand="monark",gears="7"}

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
Copy link

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);
}

@joschi joschi added this to the 5.0.0 milestone Mar 9, 2022
@github-actions github-actions bot added the Stale label Sep 6, 2022
@dropwizard dropwizard deleted a comment from github-actions bot Sep 6, 2022
@joschi joschi removed the Stale label Sep 6, 2022
@github-actions github-actions bot added the Stale label Mar 6, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 20, 2023
@joschi joschi removed the Stale label Mar 20, 2023
@dropwizard dropwizard deleted a comment from github-actions bot Mar 20, 2023
@joschi joschi reopened this Mar 20, 2023
@github-actions github-actions bot added the Stale label Sep 17, 2023
@dropwizard dropwizard deleted a comment from github-actions bot Sep 22, 2023
@joschi joschi removed the Stale label Sep 22, 2023
Copy link

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.

@github-actions github-actions bot added the Stale label Mar 21, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2024
@joschi joschi removed the Stale label Jun 9, 2024
@joschi joschi reopened this Jun 9, 2024
@mbrannstrom
Copy link
Author

@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.toString vs 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.

@github-actions github-actions bot added the Stale label Dec 8, 2024
@joschi joschi removed the Stale label Dec 8, 2024
@dropwizard dropwizard deleted a comment from github-actions bot Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants