Skip to content

Properties to set default configuration for auto-timed controller metrics #15988

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

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Feb 19, 2019

When management.metrics.web.server.autoTimeRequests is enabled (default=true), spring-boot collects metrics on controller methods even when they are not annotated with @Timed.
When this happens, created metrics are based on the default configuration values of @Timed.
Currently, there is no way to specify the default configuration to those auto-timed controller metrics.

This PR introduces two properties:

  • management.metrics.web.server.autoTimeRequestsDefaultPercentiles
  • management.metrics.web.server.autoTimeRequestsDefaultHistogram

When values are set to the above properties, they will be applied to auto-timed controller metrics that do not have explicit @Timed annotation.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 19, 2019
@ttddyy ttddyy force-pushed the configure-default-auto-timed-controller branch from 31834e2 to dac7aa6 Compare February 19, 2019 18:48
@wilkinsona wilkinsona added this to the 2.2.x milestone Feb 26, 2019
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 26, 2019
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

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

Thanks very much for the PR. I left a few comments. Could you please take a look when you have a moment and make the necessary changes if you agree?

* {@link Timed} is not presented on the corresponding request handler. Any
* {@link Timed} annotation presented will have precedence.
*/
private double[] autoTimeRequestsDefaultPercentiles;
Copy link
Member

Choose a reason for hiding this comment

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

The field javadoc is used to document the configuration property in the reference documentation appendix and in the metadata that's used by IDEs. As such, it can't use javadoc tags and must be written in plain text. Can you please rework the description to match the style of the other fields in the class?

* {@link Timed} is not presented on the corresponding request handler. Any
* {@link Timed} annotation presented will have precedence.
*/
private boolean autoTimeRequestsDefaultHistogram;
Copy link
Member

Choose a reason for hiding this comment

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

The field javadoc is used to document the configuration property in the reference documentation appendix and in the metadata that's used by IDEs. As such, it can't use javadoc tags and must be written in plain text. Can you please rework the description to match the style of the other fields in the class?

@@ -88,10 +92,31 @@ public WebMvcMetricsFilter(ApplicationContext context, MeterRegistry registry,
*/
public WebMvcMetricsFilter(MeterRegistry registry, WebMvcTagsProvider tagsProvider,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should deprecate this constructor in favour of the new one added below.

@@ -64,10 +69,19 @@ public MetricsWebFilter(MeterRegistry registry, WebFluxTagsProvider tagsProvider

public MetricsWebFilter(MeterRegistry registry, WebFluxTagsProvider tagsProvider,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should deprecate this constructor in favour of the new one added below.

@ttddyy ttddyy force-pushed the configure-default-auto-timed-controller branch from dac7aa6 to 96712be Compare February 27, 2019 01:40
@ttddyy
Copy link
Contributor Author

ttddyy commented Feb 27, 2019

Hi @wilkinsona
Updated the PR.
I was not sure for what version to put in deprecation javadoc, so I put 2.1.4.

Thanks,

@ttddyy ttddyy force-pushed the configure-default-auto-timed-controller branch 2 times, most recently from e24f26e to 85138da Compare February 28, 2019 00:11
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

I am not sure about those new properties and how they should be structure in configuration. Flagging for team attention to see what the rest of the team thinks.

* not presented on the corresponding request handler. Any @Timed annotation
* presented will have precedence.
*/
private double[] autoTimeRequestsDefaultPercentiles;
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about the double[] array, I'd rather use a List for this.

Also we now have three properties around the concept of autoTimeRequests and some keys are now rather long. Shouldn't we take the opportunity to group those in a group?

* @param tagsProvider provider for metrics tags
* @param metricName name of the metric to record
* @param autoTimeRequests if requests should be automatically timed
* @deprecated since 2.1.4 in favor of
Copy link
Member

Choose a reason for hiding this comment

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

This PR is intended to be merged on master only so that should be deprecated as of 2.2.0

@@ -133,6 +133,20 @@ public void setMaxUriTags(int maxUriTags) {
*/
private boolean autoTimeRequests = true;

/**
* Default percentiles when "autoTimeRequests=true" and @Timed annotation is
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the idea of referring one property from another property. The canonical format is not camelCase anyway.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Feb 28, 2019
@ttddyy ttddyy force-pushed the configure-default-auto-timed-controller branch from 85138da to 367e9e7 Compare March 1, 2019 00:50
@ttddyy
Copy link
Contributor Author

ttddyy commented Mar 1, 2019

Added a commit that moves auto-time-requests related properties to an independent AutoTimeRequests properties class.

@wilkinsona
Copy link
Member

We discussed this a bit and wondered if properties like management.metrics.web.server.request.autotime.default-histogram would be preferable. We'd have similar properties on the client-side beneath management.metrics.web.client.request. We'd need to deprecate some existing properties such as management.metrics.web.server.requests-metric-name and move it to something like management.metrics.web.server.request.metric-name

@ttddyy ttddyy force-pushed the configure-default-auto-timed-controller branch from 1fc2f99 to 4279d03 Compare March 25, 2019 02:40
@ttddyy
Copy link
Contributor Author

ttddyy commented Mar 25, 2019

@wilkinsona

Updated the review for server side auto-time metrics.

Updated auto-time requests related config to:

  • management.metrics.web.server.request.auto-time.enabled
  • management.metrics.web.server.request.auto-time.default-percentiles
  • management.metrics.web.server.request.auto-time.default-histogram

Updated requests-metric-name:
management.metrics.web.server.requests-metric-name
=> management.metrics.web.server.request.metric-name

Not sure for how to deprecate the entry on properties class.
For now, I just put @deprecated for management.metrics.web.server.requests-metric-name.
If it is ok to just remove them, let me know.

I'll find some time for doing client-side.

@ttddyy
Copy link
Contributor Author

ttddyy commented Mar 27, 2019

ok, updated the client side as well.

Following properties are added:

  • management.metrics.web.client.request.auto-time.enabled
  • management.metrics.web.client.request.auto-time.default-percentiles
  • management.metrics.web.client.request.auto-time.default-histogram

Also, updated "requests-metric-name":
management.metrics.web.client.requests-metric-name
=> management.metrics.web.client.request.metric-name

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Mar 27, 2019
@ttddyy ttddyy force-pushed the configure-default-auto-timed-controller branch 2 times, most recently from 7850e60 to 0367236 Compare April 10, 2019 23:34
@ttddyy
Copy link
Contributor Author

ttddyy commented Apr 11, 2019

@wilkinsona
I have incorporated the comments and force pushed the squashed commit.
Please take a look.

Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

We're almost there but there are still a few things to fix IMO. Let me know if you have time to review those, thanks!

* Name of the metric for sent requests.
*/
private String requestsMetricName = "http.client.requests";
private final Request request = new Request("http.client.requests");
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately that's not very consistent with what we want in the metadata. The default should be in code rather. Let's go with a ClientRequest that hardcodes the value as it used to in the field.

* Get name of the metric for received requests.
* @return request metric name
* @deprecated since 2.2.0 in favor of {@link Request#getMetricName()}
*/
Copy link
Member

Choose a reason for hiding this comment

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

This should have a @DeprecatedConfigurationProperty with the replacement key

* Name of the metric for received requests.
*/
private String requestsMetricName = "http.server.requests";
private final Request request = new Request("http.server.requests");
Copy link
Member

Choose a reason for hiding this comment

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

Idem as above with a ServerRequest

* Get name of the metric for received requests.
* @return request metric name
* @deprecated since 2.2.0 in favor of {@link Request#getMetricName()}
*/
Copy link
Member

Choose a reason for hiding this comment

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

This should have a @DeprecatedConfigurationProperty with the replacement key

@@ -171,6 +180,86 @@ public void setMaxUriTags(int maxUriTags) {

}

public static class Request {

public Request(String metricName) {
Copy link
Member

Choose a reason for hiding this comment

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

With 2.2 this will break as the binding will consider you want to use constructor binding with a single property. Please remove that constructor.

@@ -1801,7 +1801,7 @@ application's absolute start time
[[production-ready-metrics-spring-mvc]]
==== Spring MVC Metrics
Auto-configuration enables the instrumentation of requests handled by Spring MVC. When
`management.metrics.web.server.auto-time-requests` is `true`, this instrumentation occurs
`management.metrics.web.server.auto-time-requests.enabled` is `true`, this instrumentation occurs
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't look like the right key. request.auto-time.enabled ?

@@ -1896,7 +1896,7 @@ To customize the tags, provide a `@Bean` that implements `WebFluxTagsProvider`.
[[production-ready-metrics-jersey-server]]
==== Jersey Server Metrics
Auto-configuration enables the instrumentation of requests handled by the Jersey JAX-RS
implementation. When `management.metrics.web.server.auto-time-requests` is `true`, this
implementation. When `management.metrics.web.server.auto-time-requests.enabled` is `true`, this
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't look like the right key. request.auto-time.enabled ?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Apr 17, 2019
When `management.metrics.web.server.auto-time-requests` is enabled
(default=true), spring-boot collects metrics on controller methods even
when they are not annotated with `@Timed`.
When this happens, created metrics are based on the default configuration
values of `@Timed`.
Currently, there is no way to specify the default configuration to those
auto-timed controller metrics.

This commit adds default configurations to auto-time requests on both
client and server sides.

These properties are introduced:
- "management.metrics.web.[server|client].request.auto-time.enabled"
- "management.metrics.web.[server|client].request.auto-time.default-percentiles"
- "management.metrics.web.[server|client].request.auto-time.default-histogram"

Also, "requests-metric-name" property is updated:
old: "management.metrics.web.[server|client].requests-metric-name"
new: "management.metrics.web.[server|client].request.metric-name"

This property is removed:
- `management.metrics.web.server.auto-time-requests`
@ttddyy ttddyy force-pushed the configure-default-auto-timed-controller branch from 0367236 to 02715a3 Compare April 18, 2019 00:46
@ttddyy
Copy link
Contributor Author

ttddyy commented Apr 18, 2019

ok, incorporated the review comments and updated/rebased/squashed the commit.

@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Apr 30, 2019
@snicoll snicoll self-assigned this May 1, 2019
@snicoll snicoll removed this from the 2.2.x milestone May 1, 2019
@snicoll snicoll added this to the 2.2.0.M3 milestone May 1, 2019
snicoll pushed a commit that referenced this pull request May 1, 2019
When `management.metrics.web.server.auto-time-requests` is enabled
(default=true), Spring Boot collects metrics on controller methods even
when they are not annotated with `@Timed`.

When this happens, created metrics are based on the default
`@Timed` configuration and there is no way to customize the
configuration of those auto-timed controller metrics.

This commit adds default configurations to auto-timed requests on both
client and server sides.

See gh-15988
@snicoll snicoll closed this in 8045bf1 May 1, 2019
snicoll added a commit that referenced this pull request May 1, 2019
* pr/15988:
  Polish "Allow configuration of auto-timed metrics"
  Allow configuration of auto-timed metrics
@snicoll
Copy link
Member

snicoll commented May 1, 2019

@ttddyy thank you for your hard work on this one. This is merged on master with a polish commit.

@ttddyy ttddyy deleted the configure-default-auto-timed-controller branch May 1, 2019 06:44
@snicoll
Copy link
Member

snicoll commented May 14, 2019

I can't reopen this pull request but the auto-time-requests property has been removed rather than being deprecated and I missed that as part of the polish. I'll try to restore it in a deprecated fashion now.

snicoll added a commit that referenced this pull request May 14, 2019
This commit makes sure the "auto-time-requests" property is still
available in a deprecated fashion.

See gh-15988
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.

5 participants