Graphite documentation does not document native tags support

[snicoll]

Looking at http://micrometer.io/docs/registry/graphite it looks like the native Graphite tags support was not documented and tagsAsPrefix is still documented as the way to do things.

We could use come clarification so that the Spring Boot reference guide can point to it.

See also micrometer-metrics/micrometer#1806

[shakuzen]

/cc @fitzoh in case you are interested in updating the documentation for the feature you contributed. Also fine if not.

[fitzoh]

#124 open.

At risk of going a little off topic/repeating info you're already aware of @snicoll , there is some risk of confusion/broken monitoring when people upgrade.

This will definitely break some people's dashboards on upgrade due to new metric names, and will drop metrics completely for people using older versions (pre 1.1.0) of Graphite which don't support tags.

[snicoll]

Thanks @fitzoh.

As far as I understand, tagsAsPrefix is only used by the hierarchical naming convention. I wonder if the current arrangement is the right one. If the user has set some tags using that property, it's a bit odd they have to disable graphiteTags as well. On the other hand, if you're new to Micrometer or Graphie, you'd want the native stuff to work out of the box.

I don't know if Micrometer has done that elsewhere but the default value of that boolean flag can be conditional to the fact that at least one tag has been set on the tagsAsPrefix property. This would transparently let users upgrade if they're using this feature.

Have I missed something?

[fitzoh]

As far as I understand, tagsAsPrefix is only used by the hierarchical naming convention.

Correct, tagsAsPrefix is ignored if graphiteTagsEnabled is true.

If the user has set some tags using that property, it's a bit odd they have to disable graphiteTags as well.

Just to clarify, the tagsAsPrefix is ignored/unused if graphiteTagsEnabled is true.

I think you've got the dependencies backwards.

You can see where the configuration is used here:
https://github.com/micrometer-metrics/micrometer/blob/592f09d19e58af6106788d0c9053fd73ebcb2cc2/implementations/micrometer-registry-graphite/src/main/java/io/micrometer/graphite/GraphiteMeterRegistry.java#L33-L35

[snicoll]

I think you've got the dependencies backwards.

Have I? graphiteTagsEnabled is true by default in Micrometer. I am making the case that if you've set tagsAsPrefix this is not great as you also have to set graphiteTagsEnabled to false in order for that configuration to work.

What do you think about the proposal of inferring the default value of the flag based on the presence of tagsAsPrefix?

[fitzoh]

Ah, gotcha....
I missed the distinction between discussing how things work now vs how things should work.

So graphiteTagsEnabled() becomes

    default boolean graphiteTagsEnabled() {
        return tagsAsPrefix().length == 0;
    }

I think I like it, I definitely struggled w/ how to set defaults on the initial PR.

[snicoll]

So graphiteTagsEnabled() becomes

Ah that's interesting it doesn't expose a property right now. That looks wrong to me. This method must rather be:

default boolean graphiteTagsEnabled() {
    String v = get(prefix() + ".graphiteTagsEnabled");
    return (v != null) ? Boolean.parseBoolean(v) : tagsAsPrefix().length > 0
}

I overlooked that no property is exposed at the moment. So perhaps this is something I could fix in Spring Boot if we don't want to break things in Micrometer.

[fitzoh]

I overlooked that no property is exposed at the moment.

Oops...
Looks like tagsAsPrefix doesn't have a property either.

So perhaps this is something I could fix in Spring Boot if we don't want to break things in Micrometer.

Yeah, it's a reasonable change, but is potentially a minor breaking change?
Not sure if it would be reasonable for a micrometer patch release or would make more sense in 1.5.0

[shakuzen]

Not sure if it would be reasonable for a micrometer patch release or would make more sense in 1.5.0

Spring Boot hasn't and won't depend on 1.4.x, so a change in Micrometer 1.5.0 would be the same from the Spring Boot perspective. It's more a question of non-Boot users. Let's make the change in 1.5 so it's less surprising, and we're not so far off from the 1.5 release anyways.