Spring-boot: Auto-configure Micrometer's HumioMeterRegistry

[jkschneider]

See https://github.com/micrometer-metrics/micrometer/pull/840 and specifically HumioMetricsExportAutoConfiguration.

[wilkinsona]

The Map<String, String> property with a default value throws up an interesting scenario for the configuration property metadata. I've opened https://github.com/spring-projects/spring-boot/issues/14813 to see what we can do about that.

[snicoll]

As I’ve indicated in that issue, if there are standard tags, I’d like to see a dedicated structure with those tags listed + a map for additional ones. Hopefully that one will not have a default key in it.

[wilkinsona]

There are, AFAIK, no standard tags. They’re just arbitrary key-value pairs. I wonder if it may actually be better if there was no default and we required the user to specify one or more tags.

[snicoll]

IMO a raw map with a default value is a smell so I agree we should do something before adding this.

[wilkinsona]

I wonder if it may actually be better if there was no default and we required the user to specify one or more tags.

The user need not even do that as Humio will accept events with no tags. I'm not sure that a default #name=micrometer tag adds much value so, given that a tag isn't required, I think we should clear the default. That seems quite reasonable to me, particular given that we're using the sandbox repository by default.

WDYT, @jkschneider? Could the default for tags be changed in Micrometer?

[jkschneider]

The default needs to stay. Tags in Humio don't mean the same thing as tags in other monitoring systems. Our typical notion of a tag maps to an attribute in Humio. Tags in Humio serve to uniquely identify the datasource to which all metrics will be added.

They loosely indicate that there should not be more than several datasources per organization without presenting us with a hard limit. Adding a single key/value pair identifier serves to make all Micrometer metrics flow to one datasource, differentiating them at least from log-based ingestion, etc.

IMO a raw map with a default value is a smell

I'm open to suggestions, but in this case the contract is literally an arbitrary set of key value pairs used to uniquely identify a datasource, and a datasource _should_ be identified.

[jkschneider]

For example, here is a Humio query:

#name = "micrometer"
| name = http_server_requests
| eval(avg=sum/count)
| timechart(uri, function=[max(avg)])

• #name = "micrometer" selects the datasource
• name = http_server_requests selects the metric

Without a default "tag", there is no way to select the datasource.

[wilkinsona]

Thanks, Jon. I already understood the purpose of tags in Humio. I still don’t really understand the current default and whether or not we think it’ll serve most people’s purposes.

#name=micrometer feels like leaking an implementation detail about the source of the ingested events. As such, I imagine it’s some that many people will want to change. That’s little more than an hunch though. Either way, for those that do want to configure their own tags, things are further complicated by having to understand whether any tags that are configured via properties or yaml will add to or replace the default.

If we believe there’s a requirement for at least one tag to be configured, could the default not be an empty map and then fail-fast at startup if no tags are configured? That would guide users in the right direction, avoid confusion around replacing or adding to the default and would also deal with the case where the user provides a custom HumioConfig with no tags.

[wilkinsona]

Without a default "tag", there is no way to select the datasource.

When experimenting earlier I observed that Humio automatically adds a tag based on the name of the repository so it should be possible to select the data source even when no tags have been configured on the application side.

[wilkinsona]

things are further complicated by having to understand whether any tags that are configured via properties or yaml will add to or replace the default.

Any tags configured via the environment will add to the default. That means that you have to resort to programmatic configuration to get rid of the name tag. The best you can do in properties is specify an empty value.

In a Boot context, the default tag feels like a non-starter to me. We need to figure out the best replacement.

[jkschneider]

When experimenting earlier I observed that Humio automatically adds a tag based on the name of the repository so it should be possible to select the data source even when no tags have been configured

Understood, but then all metrics sources are dumping data into the same datasource in a given repository, which feels a little wrong.

Any tags configured via the environment will add to the default.

I don't see why this is the case. Based on the semantics of PropertiesConfigAdapter#get, don't we only use the default map if management.metrics.export.humio.tags is not set at all?

[jkschneider]

@wilkinsona Here is a test that seems to demonstrate what I suspected.

[wilkinsona]

Understood, but then all metrics sources are dumping data into the same datasource in a given repository, which feels a little wrong.

We’re also using the sandbox repository which is also probably a little wrong for any serious usage. It seems a reasonable default though. I’d argue that having no tags or requiring the user to configure a tag is similarly reasonable.

don't we only use the default map if management.metrics.export.humio.tags is not set at all?

Not as things stand. The default map with a single entry is in HumioProperties (to align its defaults with HumioConfig). It’s added to by any management.export.humio.tags.* properties. We could, perhaps, do something different so that the default is only used when no other tags are configured, but that would be at odds with how all other configuration properties work. That’s a source of confusion that we need to avoid.

[jkschneider]

The default map with a single entry is in HumioProperties (to align its defaults with HumioConfig).

Can we simply drop this default map in HumioProperties? That seems to satisfy all concerns. I thought we didn't universally add defaults to config props classes in Spring Boot anyway, but only do so where it enhances the user experience through autocompletion?

[wilkinsona]

Unfortunately not. It leaves us with all of the concerns other than having a tag by default. The remaining concerns are:

• It has a default value that is not reflected in the metadata
• Unlike any other map property, its default value is lost as soon as another entry is configured

For the consistency with the rest of Boot, I can’t see an approach other than defaulting to no tags. That will mean that the metadata is accurate and that there’s no unusual behaviour with a default entry being lost when another is configured.

Assuming that there’s no other approach, this becomes a question of whether we should require the user to configure at least one tag (failing fast if they don’t) or export anyway and rely on the implicit tags. I think I’m leaning towards the latter.

[wilkinsona]

I thought we didn't universally add defaults to config props classes in Spring Boot anyway, but only do so where it enhances the user experience through autocompletion?

Default values are (almost) always useful for that very reason. I say almost due to the problem with a map property with contents by default that reared its head here. Other than that, we try to have defaults in the properties and use tests to keep them in sync with whatever they are configuring.

[jkschneider]

Well, I suppose I'm not left with much choice. I'll drop the default in HumioConfig and conditionally not ship tags at all if the map is null or empty.

I'm not really happy. I do think the unusual behaviour of a default entry being lost when another is configured best protects the user, but integration with Spring Boot is more important than this little nod to usability.

[jkschneider]

Done with https://github.com/micrometer-metrics/micrometer/commit/1d2326f9a9b23512c275b4fb09d9902cd5daf1d1

[wilkinsona]

I'm not really happy.

That's not good.

I do think the unusual behaviour of a default entry being lost when another is configured best protects the user

Perhaps there's some better middle ground. I wonder if we should require the user to configure one or more tags? That would protect them while maintaining consistent configuration behaviour. It's not much different to other registries which have a few pieces of mandatory configuration.

[snicoll]

Couldn't we extract that tags property in an object that has a name String attribute that defaults to micrometer and then an empty Map for additional tags? That's where I was going with "known tags" earlier. This allows up to explicit document some well known tags and if Micrometer decides that they should have a default, then we could also document that.

Of course, that means that the code on the "non Spring Boot side" needs to merge the incoming map to make thinks consistent.

[wilkinsona]

No, I don't think so. There's no requirement for there to be a tag called name and many people will not want such a tag. In other words, there are no well-known tags and what you call your tags is entirely dependent on personal preference, organisational conventions, etc.

[mwl]

Without a default "tag", there is no way to select the datasource.

When experimenting earlier I observed that Humio automatically adds a tag based on the name of the repository so it should be possible to select the data source even when no tags have been configured on the application side.

Correct. When no tag filters are given in the query, Humio will search across all tags.

Although, that said. It turns out the Humio ingest api actually supports and empty object ({}) or null for tags. So in the autoconfigurer we can easily default to null without doing any damage.

[jkschneider]

So in the autoconfigurer we can easily default to null without doing any damage.

That is effectively what we've done with micrometer-metrics/micrometer@1d2326f

[mwl]

@jkschneider That's great. Although the empty check isn't necessary. Both {"tags": {}, "events": …}, {"tags": null, "events": …}, and {"events": …} are allowed.

[jkschneider]

Thanks @mwl. I'm OK sending the simplest JSON form of {"events": …}.

[mwl]

Don't know how far you've got, but I got this far https://github.com/spring-projects/spring-boot/compare/master...mwl:master

[wilkinsona]

Thanks for the offer, @mwl, but I already had this one in hand. If you are interested in the changes that I made, please take a look at https://github.com/spring-projects/spring-boot/commit/1e2d5a1382897277a0161b16984c4e705327e8d8.

[mwl]

Thanks @wilkinsona. I learned that after I've finished that you were also working on the Auto configuration. Anyway. Looks like we pretty much agree on how it's done 👍