Add shortcut to assign dynamic tags to Meters

[gavlyukovskiy]

From slack thread it would be nice to have ability to define only dynamic part in metric while metric with static tags can be defined only once:

registry.timer("my.timer", "static", "tag", "response", response.status()).record(1);

->

Timer myTimer = registry.timer("my.timer", "static", "tag")
...
myTimer.withTags("response", response.status()).record(1);

where withTags returns new instance of timer with additional tags attached.

[jkschneider]

Ingenious usability hack.

[zeratul021]

@jkschneider is there a recommended way of solving this issue?
E.g. I want to have counter for registered users and the dynamic tag would be their country of origin.
TIA

[jkschneider]

Sorry this won't get done for 1.1.0. It is actually much harder than it would initially seem. We don't habitually maintain a reference to the registry on each meter, so spawning a completely new metric is difficult. Also, for timers we don't tend to preserve all the options (like DistributionSummaryConfig) that we used to create a timer after it exists. Technically there is no way to go back and introduce this without a breaking API change.

[jkschneider]

@zeratul021 It is possible to use dynamic tags now though, simply by registering it as you use it. If the meter already exists, Micrometer will return the existing one.

[zeratul021]

@jkschneider thanks for the input. Yeah, I'm using it exactly like that. Seems there is nothing wrong with it for now.

[u6f6o]

@jkschneider if I am not mistaken, this pattern (register again) cannot be applied to gauges without building a new gauge every time? In opposite to the other meters, the registry returns the gauge value instead of a wrapper representing the gauge (which makes perfect sense imo).

I currently have to migrate a lot of existing, dynamic gauges and came up with this workaround to register dynamic gauges:

private final ConcurrentHashMap<Meter.Id, AtomicLong> dynamicGauges = new ConcurrentHashMap<>();

private void handleDynamicGauge(String meterName, String labelKey, String labelValue, Long snapshot) {
    Meter.Id id = new Meter.Id(meterName, Tags.of(labelKey, labelValue), null, null, GAUGE);

    dynamicGauges.compute(id, (key, current) -> {
        if (current == null) {
            AtomicLong initialValue = new AtomicLong(snapshot);
            registry.gauge(key.getName(), key.getTags(), initialValue);
            return initialValue;
        } else {
            current.set(snapshot);
            return current;
        }
    });
}

Is there any other way, to achieve dynamic gauges with micrometer instead of using my workaround? I am relying a bit on micrometer internals (Meter.Id equals/hashcode methods) and would be happy about any advice.

[jkschneider]

@u6f6o Version 1.1.0 has a MultiGauge that allows you to register several gauges with a common name and set of tags but that varies by some other set of tags.

[u6f6o]

@jkschneider thanks for pointing me in the right direction.

[mgr32]

I needed to add a dynamic tag for all meters that have already been registered, and I ended up with following workaround - it adds the tag in getConventionTags() which is invoked from publish() of registries (e.g. ElasticMeterRegistry):

public class DynamicValueAddingMeterFilter implements MeterFilter {

    private final DynamicValueProvider dynamicValueProvider;

    @Override
    public Meter.Id map(Meter.Id id) {
        return new DynamicMeterId(id);
    }

    class DynamicMeterId extends Meter.Id {

        DynamicMeterId(Meter.Id delegateId) {
            super(delegateId.getName(),
                  Tags.of(delegateId.getTags()),
                  delegateId.getBaseUnit(),
                  delegateId.getDescription(),
                  delegateId.getType()
            );
        }

        @Override
        public List<Tag> getConventionTags(NamingConvention namingConvention) {
            List<Tag> result = super.getConventionTags(namingConvention);
            result.add(Tag.of("myDynamicTag", dynamicValueProvider.getDynamicValue())));
            return result;
        }
    }
}	

[todaynowork]

@jkschneider thanks for the input. Yeah, I'm using it exactly like that. Seems there is nothing wrong with it for now.

Hello @zeratul021 @jkschneider , I am not quite understand how to add dynamic tags. Could you kindly sharing code block I could refer to ? My scenario is, we have a set of spring cloud stream applications to process data in kafka, from one system to other. We'd like to record metrics of each batch while most of application are running always to process data. The producer produce the data with batch id. In always running application,we'd record metrics according to batch id. So we are going to change the batch id value according the message. I am not sure if that possible. I did some tests to register new counter, but the counter come with tag of "batch_id", and values of all processed batch id . I expect just get the tag of "batch_id" and value of current batch id.

[mahesh031]

Can someone post the snippet for adding the dynamic tags to the existing meter? Also, how to register and get the meter from existing list?

[hanagiat]

https://dzone.com/articles/spring-boot-metrics-with-dynamic-tag-values - link to the source inside the article

[giteshjha]

This worked for me.
Suppose you have configured the common metrics using registry.config().commonTags() and now want to change the value of few of the tag-key at run time. You can do this -

registry.config().meterFilter(MeterFilter.replaceTagValues(
                "tag_key", s->"tag_value"
        ));

[wentfar]

Another solution for dynamic labels

[hanagiat]

Very nice!

…

On Fri, Aug 12, 2022 at 8:54 AM Ming Yang ***@***.***> wrote: Another solution for dynamic labels <https://github.com/avpines/dynamic-actuator-meters> — Reply to this email directly, view it on GitHub <#535 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4NVGP7SC6JH6QHQLXVFI3VYXRITANCNFSM4EYXREXQ> . You are receiving this because you commented.Message ID: ***@***.***>

[jonatan-ivanov]

Sorry for not getting back to this issue earlier.

Dynamic Tags

Let me try to address the dynamic tags topic first that was discussed in this issue.
I think this issue is not about how to do dynamic tagging but quite the opposite: how to define and reuse static tags. @gavlyukovskiy, please correct me if I'm wrong.
Though the good news is: you can do dynamic tags with Micrometer:

void doSomething(String status) {
    Timer.builder("my.timer")
        .tags("status", status)
        .register(registry)
        .record(() -> { /* do something here */ });
}

void doSomething2(String status) {
    Sample sample = Timer.start(registry);
    // do something here
    sample.stop(registry.timer("my.timer", "status", status));
}

Neither register nor registry.timer creates a new Timer (if not needed) they will look up an existing one (if any) and return it.

Define static tags only once

I think the original example:

registry.timer("my.timer", "static", "tag", "status", status).record(1, MILLISECONDS);

Can be rewritten in this form to fulfill the need of defining static tags once:

Tags tags = Tags.of("static", "tag");
...
registry.timer("my.timer", tags.and("status", status)).record(1, MILLISECONDS);

Which I think is pretty similar to the original proposal:

Timer timer = registry.timer("my.timer", "static", "tag")
...
timer.withTags("status", status).record(1, MILLISECONDS);

Except this one also includes the name not just the tags.
I think something similar is possible, but as @jkschneider stated in #535 (comment) it is not possible without calling register, i.e.: we can do something like this:

// in the Builder, we can create a Builder from a Timer
Builder(Timer timer) {
    super(timer.getId().getName());
    super.tags(timer.getId().getTags());
    ...
}

// this would enable us to use that through the Timer
static Builder fromTimer(Timer timer) {
    return new Builder(timer);
}

// so you can do something like this (please notice the register call)
Timer timer = registry.timer("my.timer", "static", "tag");
...
Timer.fromTimer(timer).tag("status", status).register(registry).record(1, MILLISECONDS);

or doing this with the builder:

Builder builder = Timer.builder("my.timer").tag("static", "tag");
...        
Timer.fromBuilder(builder).tag("status", status).register(registry).record(1, MILLISECONDS);

But I find these confusing and I think passing around Timers/Builders like this would also open a lot of possibilities to misuse things, I find keeping the reference to a static Tags instance is simpler:

Tags tags = Tags.of("static", "tag");
...
registry.timer("my.timer", tags.and("status", status)).record(1, MILLISECONDS);

What do you think?

[gavlyukovskiy]

Though the good news is: you can do dynamic tags with Micrometer

My original concern was that, while possible, it's very cumbersome to do that right now (as you also described).

I think this issue is not about how to do dynamic tagging but quite the opposite: how to define and reuse static tags. @gavlyukovskiy, please correct me if I'm wrong.

Let me expand my line of thoughts here. My main idea was to distinguish between static part (metric declaration with name, static tags, etc.) and dynamic part (dynamic tags and measurement value). In terms of API I like the idea of defining all the metrics in a single place (usually class fields, but sometimes a separate class with metrics only):

class MyService {
    private final Counter operations = Counter.builder("service.operations.total")
            .register(Metrics.globalRegistry);
    private final Counter errors = Counter.builder("service.errors.total")
            .register(Metrics.globalRegistry);

    void doSomething() {
        try {
            operations.increment();
            ...
        } catch (Exception e) {
            errors.increment();
        }
    }
}

I see this pattern widely used by multiple libraries and projects I've worked on as it gives nice separation between declaration with all boilerplate code and use (operations.increment()).

When we add dynamic tags, it means that we have to break that declaration and use into something like that

    void doSomething() {
        try {
            operations.increment();
            ...
        } catch (Exception e) {
            Counter.builder("service.errors.total")
                    .tags("type", e.getClass().getSimpleName())
                    .register(Metrics.globalRegistry)
                    .increment();
        }
    }

which breaks this pattern and makes code less readable with metrics scattered over the class.

Now, while the example above doesn't look that bad, when you start using more features it gets really ugly - for all our metrics we advice folks to declare description, for timers we also advice to declare minimumExpectedValue, maximumExpectedValue, publishPercentileHistogram and publishPercentiles, so that typical timer declaration looks like this

Timer.builder("cache.initialization.latency")
        .description("Time initialize cache from database")
        .publishPercentiles(0.99, 0.999)
        .publishPercentileHistogram()
        .minimumExpectedValue(Duration.ofSeconds(10))
        .maximumExpectedValue(Duration.ofSeconds(600))
        .register(registry)

which means that whenever you want to record a measurement and dynamic tag value, you also need to re-declare timer from scratch with all that configuration.

Well, I know that Meter.Id internally only uses metric and tag names in equals and hashCode, which means I could actually write this code like this

private final Timer timer = Timer.builder("cache.initialization.latency")
        .description("Time initialize cache from database")
        .tags("cacheName", "unused")
        .publishPercentiles(0.99, 0.999)
        .publishPercentileHistogram()
        .minimumExpectedValue(Duration.ofSeconds(10))
        .maximumExpectedValue(Duration.ofSeconds(600))
        .register(registry)

void doSomething(String cacheName) {
    ...
    Timer.builder("cache.initialization.latency")
            .tags("cacheName", cacheName)
            .register(Metrics.globalRegistry)
            .record(latency);
}

this still requires more boilerplate than it has to, produces one useless dimension (cacheName=unused), requires to know how register works internally and is not apparent for many engineers new to Micrometer - as a personal anecdote there were at least a dozen times people were using cache to store metrics for different tag values to avoid "costly register", while I had to explain that register is exactly that - a map with metric name + tag names as a key.
Out of curiosity I just searched Github for "micrometer computeIfAbsent" and found 1, 2, 3 falling into the same trap.

Let's compare that to prometheus client:

private final Summary timer = Summary.build()
        .name("cache_initialization_latency")
        .labelNames("cacheName")
        .help("Time initialize cache from database")
        .register();

void doSomething(String cacheName) {
    ...
    timer.labels(cacheName).observe(latency);
}

which offers ideal separation between static configuration and dynamic values.

@jonatan-ivanov I hope that makes problem statement more clear. As for the solution - I don't know how to make hypothetical withTags work, since Meter has no reference to a registry (as you and @jkschneider mentioned).

In Kotlin I've been using this for a while:

class MyService(registry: MeterRegistry) {
    private val cacheInitializationTimer = { cacheName: String ->
        Timer.builder("cache.initialization.latency")
            .tags("cacheName", cacheName)
            .description("Time initialize cache from database")
            .publishPercentiles(0.99, 0.999)
            .publishPercentileHistogram()
            .minimumExpectedValue(Duration.ofSeconds(10))
            .maximumExpectedValue(Duration.ofSeconds(600))
            .register(registry)
    }

    fun doSomething(cacheName: String) {
        ...
        cacheInitializationTimer(cacheName).record(latency)
    }
}

which achieves minimal verbosity by capturing a reference to registry inside function body (so you don't have to keep it in the field), is used uniformly with other metrics without dynamic tags - cacheInitializationTimer(cacheName).record(latency) vs timer.record(latency), is type-safe and can utilize named parameters for extra clarity (cacheInitializationTimer(cacheName = "mycache").record(latency))

The builder you suggested

Timer.fromTimer(timer).tag("status", status).register(registry).record(1, MILLISECONDS);

is probably the closest one to what I had in mind as it would be able to capture static tags and other configuration, but still quite verbose. Alternatively one could simply not register the timer and store a reference to a builder: (EDIT: won't work as builder is mutable and needs to be copied)

private final Timer.Builder timer = Timer.builder("cache.initialization.latency")
        .publishPercentiles(0.99, 0.999)
        .publishPercentileHistogram()
        .minimumExpectedValue(Duration.ofSeconds(10))
        .maximumExpectedValue(Duration.ofSeconds(600));

void doSomething(String cacheName) {
    timer.tags("cacheName", cacheName).register(registry).record(1.0);
}

or a method that returns a timer:

Timer cacheInitializationTimer(String cacheName) {...}

[shakuzen]

as a personal anecdote there were at least a dozen times people were using cache to store metrics for different tag values to avoid "costly register", while I had to explain that register is exactly that - a map with metric name + tag names as a key.

The Micrometer team has seen this as well. Ideally I think we would go back in time and name the method something more verbose but clear, such as registerOrRetrieve, to avoid this common confusion. We could try to introduce such a method and migrate users to it, but I'm afraid it would cause a lot of work for people if we ever deprecated register in favor of that method. And if we leave both methods, it could create confusion that there's a difference between them and each should be used in different cases.

Let's compare that to prometheus client which offers ideal separation between static configuration and dynamic values.

I always thought the opportunity to assign a label value to the wrong label name and it not be obvious in code was a problem in the Prometheus Java client API. With a single label, of course you can't get it wrong, but as soon as you have two or more, and since the declaration and recording are separated, you have no idea at the recording site what order to give the label values (or even how many there are) without going back to reference the declaration. I think it was intentional in Micrometer's API design to avoid this type of issue by always taking the name and value together. I think the example you gave with Kotlin helps work around this nicely by having named parameters, but that is an optional feature still and exclusive to Kotlin.

You raise some good points, and we're open to improving usability of the API. It's a challenging problem, though, and we have to be careful to not increase complexity for new users with even more ways to achieve the same result.

[jonatan-ivanov]

Thank you for the detailed answer, this helps a lot.
What do you think about this:
(This is hypothetical, I haven't tried copying Timer.Builder and DistributionStatisticConfig.Builder this deeply.)

// we can add this to the Timer: static Timer with(Builder builder) {...}

Timer.Builder builder = Timer.builder("cache.initialization.latency")
    .description("Time initialize cache from database")
    .tag("static", "abc")
    .publishPercentiles(0.99, 0.999)
    .publishPercentileHistogram()
    .minimumExpectedValue(Duration.ofSeconds(10))
    .maximumExpectedValue(Duration.ofSeconds(600));

[...]

void doSomething(String cacheName) {
    Timer.with(builder)
        .tags("cacheName", cacheName)
        .register(registry)
        .record(latency);
}

We can also add overloads to the with method so that you can do:

// static Timer with(Builder builder, Tags tags, MeterRegistry registry) {...}
Timer.with(builder, Tags.of("cacheName", cacheName), registry).record(latency);

// or with a "factory": static Function<Tags, Timer> with(Builder builder, MeterRegistry registry) {...}
Timer.with(builder, registry).apply(Tags.of("cacheName", cacheName)).record(latency);

Unfortunately right now I don't see any way working around the fact that Timer does not have a reference to the MeterRegistry (and I think it should not have it).

EDIT:
I implemented them but after played with them a little, to me they kind of feel the same:

builder().tags(Tags.of("cacheName", cacheName)).register(registry).record(latency);
Timer.with(builder, Tags.of("cacheName", cacheName), registry).record(latency);
Timer.with(builder, registry).apply(Tags.of("cacheName", cacheName)).record(latency);

[jonatan-ivanov]

After playing a little more, with the "factory" approach above , you can do this:

// Timer.Builder has this: Function<Tags, Timer> with(MeterRegistry registry);

Function<Tags, Timer> factory = Timer.builder("cache.initialization.latency")
    .description("Time initialize cache from database")
    .tag("static", "abc")
    .publishPercentiles(0.99, 0.999)
    .publishPercentileHistogram()
    .minimumExpectedValue(Duration.ofSeconds(10))
    .maximumExpectedValue(Duration.ofSeconds(600))
    .with(registry);

void doSomething(String cacheName) {
    factory.apply(Tags.of("cacheName", cacheName)).record(latency);
}

See: jonatan-ivanov@f47c83b

[gavlyukovskiy]

I always thought the opportunity to assign a label value to the wrong label name and it not be obvious in code was a problem in the Prometheus Java client API

+1 to this.

factory.apply(Tags.of("cacheName", cacheName)).record(latency);

@jonatan-ivanov I really like this factory approach.

[jonatan-ivanov]

If you are following this issue, we would appreciate your feedback on this: #4097 (comment)

[shakuzen]

This was our most highly requested feature, and it will be in the upcoming 1.12.0-RC1 release. Please try it out before our 1.12.0 GA release next month. We'd love to get your feedback so we can make sure this is as useful as possible.