Skip to content

Conversation

@shakuzen
Copy link
Member

(Hopefully) makes it more clear that the open and max part of the metric refers to file descriptors. This may also help querying these metrics since their prefix is now the same.

The FileDescriptorMetrics are also never being used, but I will send a separate pull request for that.

@mweirauch
Copy link
Contributor

I initially implemented it like that due to the IMHO better hierarchical naming. Then there was some discussion regarding "Prometheus client implementation" conformance in #175 and #170.

In the meantime I've seen different projects exposing Prometheus metrics not adhering to any Prometheus-proposed naming conventions I whonder if it's really worth it. In one comment I said that Micrometer is the system, not that one registry implementation for Prometheus.

Anyways, I consider the metric names part of the API, so with whatever we go with should be stable before 1.0.0 hits the road.

@shakuzen
Copy link
Member Author

@mweirauch Thank you for sharing the background, and my apologies I didn't search to find the previous discussion before proposing the changes.

In one comment I said that Micrometer is the system, not that one registry implementation for Prometheus.

I think I agree, and maybe we are better off registering a filter when Prometheus registry implementation is used that renames the metrics that don't match what Micrometer otherwise prefers to call them. This seems more practical since other systems will likely have their own preferences.

Anyways, I consider the metric names part of the API, so with whatever we go with should be stable before 1.0.0 hits the road.

I agree with this also.

@jkschneider
Copy link
Contributor

Agree with all of the above. And this is a good idea:

maybe we are better off registering a filter when Prometheus registry implementation is used that renames the metrics that don't match what Micrometer otherwise prefers to call them

(Hopefully) makes it more clear that the `open` and `max` part of the metric refers to file descriptors. This may also help querying these metrics since their prefix is now the same.
@shakuzen shakuzen force-pushed the file-descriptors-metric-name-fix branch from 7ad950a to 6169b27 Compare February 13, 2018 10:09
@wilkinsona
Copy link
Contributor

I originally opened #425, but @shakuzen pointed out that it was largely a duplicate of this PR so I've closed it.

In addition to the renaming proposed here, could changing fds to something more descriptive also be considered? Perhaps files:

  • process.files.open
  • process.files.max

Where Prometheus has an opinion about the metric name it receives for a specific metric and it differs from Micrometer's generic preferred naming, the `PrometheusMetricRenameFilter` will convert the metric name. This allows Micrometer to use its preferred metric name but cater to Prometheus where it expects something specific, without affecting other registries.
@shakuzen shakuzen force-pushed the file-descriptors-metric-name-fix branch from 3939d34 to 87a6ac4 Compare February 13, 2018 12:15
@shakuzen
Copy link
Member Author

@jkschneider I have added a PrometheusMetricRenameFilter and configured it in auto-configuration. Let me know if this is what you had in mind. I can also apply @wilkinsona's suggested name change to process.files.* if you agree.

@mweirauch
Copy link
Contributor

Must admit, I like the renaming proposal to process.files.*. What I somehow wouldn't like to see is a metric renaming just to be "compliant" with the docs. IMHO it's rather unintuitve that we rename it. Leads to confusion why it's this in registry A, and that in registry B.

If a language or runtime doesn't expose one of the variables it'd just not export it.

Let's say we "don't export it" (officially) and we stick to the (re-proposed) hierarchical name?

Adding to that, I don't see any big overlaps in metric naming e.g. between Micrometer, Dropwizard and Prometheus-Java. These are heterogenous per-se.

@checketts
Copy link
Contributor

I also like the renaming of filedescriptors to files. I think the PrometheusMetricRenameFilter is a good idea to have. I'm also OK if it is off by default.

While it is nice to have metrics remain the same between languages (go and java apps for example), they truly diverge very quickly (tracking coroutines versus threads for example). Keeping them the same is only helpful if the metric names are non-intuitive to begin with (fds for example)

jkschneider pushed a commit that referenced this pull request Feb 13, 2018
@jkschneider jkschneider added the doc-update A documentation update label Feb 13, 2018
@jkschneider
Copy link
Contributor

jkschneider commented Feb 13, 2018

I think we'll leave the rename filter off by default to hopefully lessen the surprise, but document its presence for those that care to follow the Prometheus "standard" names. Glad it exists now, great idea!

Accepted rename to process.files.*.

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

Labels

doc-update A documentation update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants