Open pbetkier opened 6 years ago
This is a really clever idea @pbetkier. I'm curious to see what your naming convention looks like. How do you implement name
to fold in tags when the name
signature only provides you the metric name, type, and base unit?
My initial reaction is that I don't think we'd want to go back and add placeholders to built-in metrics for a couple reasons:
I think what we could do is provide a Placeholders
utility in Micrometer core that does a couple things. It allows you to define the mapping from OOTB names to a placeholder name. For example, in constructing a Placeholders
instance, you could define that you want jvm.memory.used
to be replaced by jvm.memory.{area}.used
. When that Placeholders
instance is bound to a particular registry, it adds a MeterFilter
that maps the names and adds the placeholder-aware naming convention to the registry.
It may be reasonable and in fact beneficial for us to provide a default GraphitePlaceholders
, because we could then publish a Grafana dash that demonstrates how to most effectively chart OOTB metrics. But folks are still free to use their own Placeholders
if they too are trying to meet an existing internal standard.
Thoughts?
Glad you like the idea :)
Our naming convention simply removes the placeholders:
public class OurPrometheusNamingConvention extends PrometheusNamingConvention {
private static final String PLACEHOLDER = "\\{[A-Za-z0-9_\\-\\.]+\\}";
@Override
public String name(String name, Meter.Type type, @Nullable String baseUnit) {
String sanitizedName = name.replaceAll("\\." + PLACEHOLDER, "");
return super.name(sanitizedName, type, baseUnit);
}
}
Resolving placeholders with tags happens in HierarchicalNameMapper
implementation that is provided for DropwizardMeterRegistry
:
public class OurHierarchicalNameMapper implements HierarchicalNameMapper {
@Override
public String toHierarchicalName(Meter.Id id, NamingConvention convention) {
String name = id.getName();
// probably could be optimized for performance, works fine for us now
for (Tag tag : id.getTags()) {
name = name.replace("{" + tag.getKey() + "}", tag.getValue());
}
if (name.contains("{") || name.contains("}")) {
throw new IllegalArgumentException("Some placeholders in the metric name do not have a matching tag! " +
"Metric name: " + id.getName() + ", after resolving with tags provided: " + name );
}
return name;
}
}
Note that our implementations may not catch all the corner-cases yet.
I agree it's better from the Micrometer project point of view not to include placeholders mechanism in the project core, but as an opt-in possibility. I like the idea of a Placeholders
class that is the entry-point for this feature and properly configures provided registries if requested explicitly. How to setup placeholders-aware HierarchicalNameMapper
though? It's now configured in DropwizardMeterRegistry
constructor.
Also, note that in our implementation of building a hierarchical metric name we don't encode tags without matching placeholders. So you have to make sure all the metrics with tags in your application are mapped to names with placeholders or else you risk getting failures to register e.g. gauge due to duplicates. Whenever you decide to drop-in out-of-the-box metrics for some tool you should know what metrics are reported and map them accordingly.
I could prepare a PR once we agree on the design.
We already have a builder type for StatsdMeterRegistry
to cover the more complex configurations like custom line builders. This isn't so different. I can imagine such a builder for hierarchical registries containing an input for Placeholders
.
So you have to make sure all the metrics with tags in your application are mapped to names with placeholders or else you risk getting failures to register
Good point. The risk is limited to gauges, function counters, and function timers which can only be described by one function. Countrs, timers, summaries would be fine.
OK. Shall I prepare a PR to discuss?
Sure, some form of this should make it into 1.1 I think.
Sorry for the delay in reviewing the pull request and getting this in a release. I've optimistically marked this for 1.2 so we can review it when merging changes for that.
How about moving forward with this? We're still using the described mechanism in our microservices and it works for us. The PR I created for discussion needs refreshing. I can do that, but I need to know if you're still interested in making this change.
@pbetkier Thanks for the reminder. My current sense is that the whole thing can be accomplished with just a PlaceholderHierarchicalNameMapper
, the construction of which defines the mappings. Something like:
PlaceholderHierarchicalNameMapper.builder()
.placeholder(MeterFilter.rename("jvm.memory.used", "process.jvm.memory.heap.used"))
.placeholder(MeterFilter.rename("http.server.requests", "api-requests.{controller}.{handler}.{method}.{code}"))
.build();
It's probably useful to define the placeholder mapping in terms of the whole Meter.Id
such as the MeterFilter#map
method does (so you can respond to base unit text, tags, etc.). There might be some new convenience methods to add to MeterFilter
such as the rename(from, to)
one hypothesized above.
What about MeterFilter
? Do we suppose that any name and tag mappings occurred before the PlaceholderHierarchicalNameMapper
kicks in you think?
I like your idea. I think it makes sense to contain this feature in only one optional class as opposed to making placeholders a global feature which impacts more of micrometer's codebase. Especially given how few metric backends are hierarchical in nature. I think it's less convenient for the Application code in process of migration from Graphite to Prometheus
case than my original idea, but we can consider it a rare use case.
I'm not sure about configuring PlaceholderHierarchicalNameMapper
with MeterFilter
objects though:
MeterFilter.rename()
and a MeterFilter.map()
which changes metric names, then configuration order matters and it may be confusing.MeterFilters
that serve a different role. Perhaps a mapping should have a domain class of its own.What are your thoughts? And what do you think about this instead?
PlaceholderHierarchicalNameMapper.Mapping mappingObject = ...;
// potentially defined as a Spring bean and constructed using
// PlaceholderHierarchicalNameMapper.Mapping.of(String,String)
// PlaceholderHierarchicalNameMapper.Mapping.from(Map<String,String>)
PlaceholderHierarchicalNameMapper.builder()
.mapping("jvm.memory.used", "jvm.memory.{area}.used") // for convenience
.mapping(mappingObject)
.build();
As a side note, I would argue that the mapping of jvm.memory.used
into process.jvm.memory.used
should be implemented by a user with a global MeterFilter
, not in a HierarchicalNameMapper
configuration.
Hello, I'd like to propose an alternative way to create metric names in backends like Dropwizard, which don't support tags natively. Either to consider implementing it in Micrometer or as a suggestion for other developers on how to solve this problem in their systems.
We've been using Dropwizard metrics reported to Graphite in our company for a couple of years. Now we're moving to Micrometer as part of Spring Boot 2 migration, also adding Prometheus as an alternative storage. I describe problems we faced and our solution.
Issues when migrating to Micrometer
One of our concerns during migration was not to change Graphite metric paths when migrating from Spring Boot 1 on Dropwizard to Spring Boot 2 on Micrometer. Some exemplary metric paths were:
Introducing tags in vanilla Micrometer results in the following:
Which brings a couple of problems:
controller
->handler
->method
->code
(captured in #595).api-requests.SomeController.someHandler.GET.200
is clear enough, we don't needcontroller
,handler
,method
andcode
segments.Controlling path encoding with placeholders
Our approach is to give control over creating the metric path to the developer instead of relying on encoding logic in Micrometer. We provide our own
HierarchicalNameMapper
andPrometheusNamingConvention
implementations which support placeholders in metric names:Our
HierarchicalNameMapper
implementation replaces all placeholders with their matching tags, mapping to our previous Graphite structure:Our
PrometheusNamingConvention
removes all placeholders from metric name, mapping to the same name as just after introducing Micrometer:Where to use placeholders
We use placeholders in all our code that is expected to support both Graphite and Prometheus. Either:
If an application doesn't need to report to Graphite and Prometheus simultaneously – either it's not using Prometheus yet or it's already completely migrated from Graphite – then placeholders are not required. When using Graphite only the application can define its metric names explicitly and ignore the tags argument. When using Prometheus only the application can use Micrometer API as it was designed.
Adoption
We started adopting this solution in our ~400 microservices stack. We register all our metrics from internal libraries using the placeholders mechanism and have a few services in the process of migration from Graphite to Prometheus also registering their metrics this way.