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
Limit metrics collection of incoming requests #14173
Conversation
6904c10
to
75733ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR @nosan!
I've added a few suggestions and am looking forward to your feedback on them. In particular, I'd like this PR to be minimal considering we target a maintenance release.
/** | ||
* {@link MeterFilter} to log only once a warning message and deny {@link Meter.Id}. | ||
* | ||
* @author Dmytro Nosan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a @since
. I wonder if that wouldn't be something of interest for the library (ping @jkschneider)
@@ -90,7 +91,7 @@ private MetricsRun() { | |||
* implementation. | |||
* @return the function to apply | |||
*/ | |||
public static Function<ApplicationContextRunner, ApplicationContextRunner> simple() { | |||
public static <T extends AbstractApplicationContextRunner> Function<T, T> simple() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not strictly related to the scope of this PR. I am happy to revisit the tests here but I'd like to do that in a separate PR.
Could you please revert everything that's related to that?
(also if submit a new PR for this, there is a raw type use of AbstractApplicationContextRunner
)
|
||
/** | ||
* Tests for {@link WebMvcMetricsAutoConfiguration}. | ||
* | ||
* @author Andy Wilkinson | ||
* @author Dmytro Nosan | ||
*/ | ||
public class WebMvcMetricsAutoConfigurationTests { | ||
|
||
private WebApplicationContextRunner contextRunner = new WebApplicationContextRunner() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I prefer the former. I'd rather keep the auto-configurations explicit and only add what's strictly necessary for the task at hand (anyway, to be discussed in a separate PR anyway).
@@ -1489,6 +1489,8 @@ content into your application. Rather, pick only the properties that you need. | |||
management.metrics.web.client.requests-metric-name=http.client.requests # Name of the metric for sent requests. | |||
management.metrics.web.server.auto-time-requests=true # Whether requests handled by Spring MVC or WebFlux should be automatically timed. | |||
management.metrics.web.server.requests-metric-name=http.server.requests # Name of the metric for received requests. | |||
management.metrics.web.server.max-uri-tags=100 # Maximum number of unique URI tag values allowed. After the max number of tag values is reached, metrics with additional tag values are denied by filter. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra line to be removed here please.
@@ -1489,6 +1489,8 @@ content into your application. Rather, pick only the properties that you need. | |||
management.metrics.web.client.requests-metric-name=http.client.requests # Name of the metric for sent requests. | |||
management.metrics.web.server.auto-time-requests=true # Whether requests handled by Spring MVC or WebFlux should be automatically timed. | |||
management.metrics.web.server.requests-metric-name=http.server.requests # Name of the metric for received requests. | |||
management.metrics.web.server.max-uri-tags=100 # Maximum number of unique URI tag values allowed. After the max number of tag values is reached, metrics with additional tag values are denied by filter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please order the key alphabetically.
3abd033
to
f55065b
Compare
Thanks, @snicoll |
@nosan thanks for the feedback but I still see some unrelated changes in If you rebase to a single commit, then a diff should show a focused minimal change for this test as well. Thanks a lot for your efforts! |
@snicoll |
@snicoll I see that |
@nosan ah ah that's awesome, I just saw that as well 5 min ago. I think so yeah, looks like an oversight. This should be done as a separate issue/PR though. Do you want to give that a try? (I am almost done with this one). |
@snicoll I'll create a separate |
* pr/14173: Polish "Limit metrics collection of incoming requests" Limit metrics collection of incoming requests
@nosan I am confused, there is a test for this in |
@snicoll RestTemplateMetricsAutoConfiguration
|
I don't understand why you refer to it. Metrics support for |
I'm talking about 'uri' tag filter. If you remove |
see gh-13913.