Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace hierarchical metrics with micrometer-based dimensional metrics #9970

Closed
wants to merge 2 commits into from
Closed

Replace hierarchical metrics with micrometer-based dimensional metrics #9970

wants to merge 2 commits into from

Conversation

jkschneider
Copy link
Contributor

@jkschneider jkschneider commented Aug 7, 2017

Let this be a starting point for discussion. Currently:

  1. I'm doing pre 1.0 releases of micrometer every several days. The first milestone release of micrometer can correspond to the Boot 2 M4 release.
  2. See the Micrometer manual for documentation about what Spring integration looks like. These docs should eventually work their way into the Spring Boot reference manual as well.
  3. This favors @EnableAtlasMetrics, etc instead of classpath-based autoconfiguration because support for several monitoring systems are based on the same dependency (spectator-api).
  4. I'll add back in the Webflux support soon! @smaldini
  5. Micrometer itself is lacking a few pieces of instrumentation that existed in actuator before (for example caffeine caches), but this gap will be closed before M4.

I expect many changes, but look forward to your feedback!

cc / @philwebb @mbhave

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 7, 2017
@jkschneider
Copy link
Contributor Author

Dependencies and autoconfiguration

The "quickstart" for Spring Boot is currently:

  1. Add @EnablePrometheusMetrics to your app.
  2. Add io.micrometer:micrometer-prometheus-starter as a dependency.

We could drop the annotation and add a marker interface in each starter module for use in autoconfiguration, but I'm a bit concerned about what happens if more than one starter winds up on the classpath. Open to suggestions on this.

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think activation was done on a user provided bean.

<!-- Optional -->
<dependency>
<groupId>io.prometheus</groupId>
<artifactId>simpleclient_common</artifactId>
<version>0.0.26</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Versions probably go in spring-boot-dependencies

import org.springframework.boot.actuate.endpoint.RequestMappingEndpoint;
import org.springframework.boot.actuate.endpoint.ShutdownEndpoint;
import org.springframework.boot.actuate.endpoint.TraceEndpoint;
import org.springframework.boot.actuate.endpoint.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* imports :-)

@wilkinsona
Copy link
Member

wilkinsona commented Aug 9, 2017

Thanks, @jkschneider. This is great stuff. I'm excited to get it into Boot. It's going to take a while to digest, particularly as half the team are on holiday. In the meantime, here's a few general comments:

  1. All copyright headers should look like this:
/*
 * Copyright 2012-2017 the original author or authors.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
  1. We use constructor injection everywhere, even in configuration classes
  2. If you want to inject something that's @Autowired(required=false) we constructor inject an ObjectProvider<Whatever> instead
  3. All new public classes should be @since 2.0.0

You can fix these if you're so inclined, or we can take care of them when we merge this. I just mention them now as I noticed them on a first read through of the changes.

@deki
Copy link
Contributor

deki commented Aug 11, 2017

Will this be part of 2.0.0?
As #5875 was dropped I would appreciate it :-)

@wilkinsona
Copy link
Member

@deki Yes, it will. I wouldn't have closed #5875 if we didn't plan to get this into 2.0

@wilkinsona wilkinsona self-assigned this Aug 30, 2017
@wilkinsona
Copy link
Member

I've started a more in-depth review and polishing of these changes here. It's still very much a work in progress.

The biggest change and potential issue that I've seen thus far is that with the Micrometer-based metrics we lose the metrics endpoint and, I think, the ability for an app to be self-sufficient in terms of storing and "serving" its own metrics. That functionality isn't much use in a serious production environment, but I suspect that people will miss it elsewhere.

@jkschneider You've described Micrometer as "think SLF4J, but for metrics". Is there an equivalent of slf4j-simple that I've missed?

@jkschneider
Copy link
Contributor Author

jkschneider commented Aug 30, 2017

@wilkinsona The boot configuration has changed quite a bit in preparation for Micrometer release 0.11.0. Once that release goes out, I'll update this PR.

There is a SimpleMeterRegistry, yes.

I've talked with a number of folks about the metrics endpoint. My opinion currently is it isn't going to really be possible to create a meaningful information dump in JSON from dimensional metrics. For the single instance self-service route, Netflix uses JMX, and Micrometer packs with a JMX registry that flattens dimensions to a hierarchical name and ships them there. We should talk more about this!

@jkschneider
Copy link
Contributor Author

It may be more productive when reviewing to review the contents of micrometer-spring-legacy and imagine that reactive support is added.

@wilkinsona
Copy link
Member

The boot configuration has changed quite a bit in preparation for Micrometer release 0.11.0. Once that release goes out, I'll update this PR

Cool, thank you. If possible, can you update the PR on top of my branch please?

@wilkinsona
Copy link
Member

wilkinsona commented Aug 31, 2017

For the single instance self-service route, Netflix uses JMX, and Micrometer packs with a JMX registry that flattens dimensions to a hierarchical name and ships them there. We should talk more about this!

My, perhaps naive, reaction to that is that if we can flatten the dimensions to a hierarchical name and ship them over JMX then we should be able to do the same for HTTP/JSON. Failing that, a single endpoint that returns a JSON payload containing all the metrics may be better than nothing.

@wilkinsona wilkinsona added this to the 2.0.0.M4 milestone Sep 14, 2017
@wilkinsona wilkinsona added priority: high type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 14, 2017
snicoll added a commit that referenced this pull request Sep 14, 2017
wilkinsona added a commit that referenced this pull request Sep 14, 2017
philwebb added a commit that referenced this pull request Sep 14, 2017
Update `MetricsAutoConfiguration` to use an `ObjectProvider` to guard
against missing beans.

See gh-9970
philwebb added a commit that referenced this pull request Sep 14, 2017
Update `MetricsClientHttpRequestInterceptor` to use `percentilesTime`
rather than `percentiles`.

See gh-9970
philwebb added a commit to philwebb/spring-boot that referenced this pull request Sep 15, 2017
Update configuration property classes used with micrometer so that
they no longer directly implement `Config` interfaces. Properties
are now adapted to Config implementations independently.

See spring-projectsgh-9970
philwebb added a commit that referenced this pull request Sep 15, 2017
Update configuration property classes used with micrometer so that
they no longer directly implement `Config` interfaces. Properties
are now adapted to Config implementations independently.

See gh-9970
@manderson23
Copy link

After this change are the Micrometer metrics reported via the /metrics actuator endpoint?

@jkschneider
Copy link
Contributor Author

jkschneider commented Sep 22, 2017

@manderson23 Yes, though because of the switch to a dimensional system, the format necessarily changed. There are some inadequacies in the /metrics endpoint that shipped with Boot 2 M4 that will be addressed in the M5 release (micrometer 1.0.0-rc.2) next week. See micrometer-metrics/micrometer#124.

@ryonday
Copy link

ryonday commented Oct 18, 2017

Is there going to be a migration guide for those of us who have, say, CounterService injected throughout our applications? It looks like right now we're being left to twist in the wind, especially since the snapshot docs still refer to CounterService and GaugeService everywhere.

@wilkinsona
Copy link
Member

@ryonday Please comment on #10313 if there's anything specific that you'd like to see in the documentation on upgrading from 1.x.

especially since the snapshot docs still refer to CounterService and GaugeService everywhere.

I can't find any instances of CounterService or GaugeService in https://docs.spring.io/spring-boot/docs/current-SNAPSHOT/reference/htmlsingle/. Can you please link to some examples?

@ryonday
Copy link

ryonday commented Oct 18, 2017

@wilkinsona Thanks for the quick response!

For example: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-actuator/README.adoc line 41 maintains the CounterService and GaugeService references (though not under these names).

It's unclear from the 2.0 documentation the prescribed method of recording one's own metrics (the section was removed from the docs). Is the expectation that we're use Micrometer directly within our applications? the CounterService and GaugeService interfaces offered a way to replace implementations while using only Spring interfaces.

@wilkinsona
Copy link
Member

Thanks. I forgot about the README when updating the documentation. I've opened #10686.

Is the expectation that we're use Micrometer directly within our applications?

Yes, that's right. Micrometer's API is a replacement for CounterService, GaugeService, and friends.

the CounterService and GaugeService interfaces offered a way to replace implementations while using only Spring interfaces.

Micrometer has the same goal. It's led by a member of the Spring team (@jkschneider) and aims to be SLF4J but for metrics. You can use the MeterRegistry interface to register things and Micrometer will take care of mapping that onto whatever metrics backend(s) you're using.

@jkschneider
Copy link
Contributor Author

@ryonday There wasn't much we could do to preserve the original CounterService and GaugeService interfaces, since they assumed a hierarchical representation of metric names and there is no reliable mapping from hierarchical names onto a dimensional scheme. The converse is possible, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants