Add support for easily disabling metrics export #21658

onobc · 2020-06-03T03:32:53Z

Add global metrics exporter disable property; set during tests with ability to override via @AutoConfigureMetrics.

onobc · 2020-06-03T03:35:16Z

...ork/boot/test/autoconfigure/metrics/ExcludeMetricExportersContextCustomizerFactoryTests.java

+ *
+ * @author Chris Bono
+ */
+class ExcludeMetricExportersContextCustomizerFactoryTests {


This test and the sibling ones AutoConfigureMetricsMissing/PresentIntegrationTest overlap on coverage. If I choose between them I think that the integration test ones are more valuable as they verify against @SpringBootTest as the annotated class.

onobc · 2020-06-03T03:36:52Z

...ate/autoconfigure/metrics/export/appoptics/AppOpticsMetricsExportAutoConfigurationTests.java

@@ -52,7 +52,14 @@ void autoConfiguresItsConfigAndMeterRegistry() {
 	}

 	@Test
-	void autoConfigurationCanBeDisabled() {
+	void autoConfigurationCanBeDisabledWithGlobalEnabledProperty() {


I will do this same mod to the other export config tests once I know we are good w/ the direction of this proposal.

onobc · 2020-06-03T03:40:05Z

@snicoll here is what I captured along the way while trying different approaches.

Background

Metircs auto-configuration topology (wrt AutoConfigureBefore/After of each other)

MetricsAutoConfiguration	
	WavefrontMetricsExportAutoConfiguration (enabled!=false -> WavefrontMeterRegistry)
	DatadogMetricsExportAutoConfiguration (enabled!=false -> DatadogMeterRegistry)
	..
		SimpleMetricsExportAutoConfiguration (COMB-MR, enabled!=false -> SimpleMeterRegistry)
			CompositeMeterRegistryAutoConfiguration	
					- NoOpMeterRegistryConfiguration (COMB-MR -> CompositeMeterRegistry empty)
					- CompositeMeterRegistryConfiguration (multi MRs -> CompositeMeterRegistry w/ MRs) 
				MetricsEndpointAutoConfiguration

Goal

To disable everything between MetricsAutoConfiguration and SimpleMetricsExportAutoConfiguration by default in tests. With ability to override and re-enable all/individual exporters.

Approaches

I tried several approaches leveraging ImportAutoConfiguration and built-in auto-config exclusion etc. but they all ended w/ non-success or non-starters shortcomings.

TLDR; Jump to approach #5 to see the content of this code proposal.

The following are of the "Don't modify any of the existing exporter auto-configurations" variety:

Approach #1:

@AutoConfigureMetrics as an @ImportAutoConfiguration which maps to list of ACs to use from spring.factories
AutoConfigurationImportFilter that exclude ACs ending in "MetricsExportAutoConfiguration"
Status: Does not work. The ACIF exclusions take precedence over the IAC list.

Approach #2:

@AutoConfigureMetrics as an @ImportAutoConfiguration which maps to list of ACs to use from spring.factories
ContextCustomizer that sets the 'spring.autoconfigure.exclude' property to list of ACs from spring.factories (above)
Status: Does not work. The exclusion always overrides the inclusions.

Approach #3:

@AutoConfigureMetrics simple marker interface, nothing more
ContextCustomizer that sets 'spring.autoconfigure.metric-exporters.enabled' based on whether the test is marked as @AutoConfigureMetrics
AutoConfigurationImportFilter that exclude ACs ending in "MetricsExportAutoConfiguration" unless above property is set (aka marked w/ @AutoConfigureMetrics)

Status: works

Pros:
- simple to implement (no change to existing auto-configs)
- users custom metric exporters will be respected IF named properly
Cons:
- excludes by naming scheme ("*MetricExportAutoConfiguration")
- binary enablement (all or nothing) - no way to override and add an inclusion/exclusion

Approach #4:

@AutoConfigureMetrics simple marker interface, nothing more
ContextCustomizer that sets the 'spring.autoconfigure.exclude' property to hardcoded set of metric exporter configs unless the test is marked w/ @AutoConfigureMetrics

Status: works

Pros:
- simple to implement (no change to existing auto-configs)
Cons:
- excludes by naming scheme ("*MetricExportAutoConfiguration") (NOTE: this can be alleviated by reading list from spring.factories)
- binary enablement (all or nothing)
- users custom metric exporters will not be respected in exclusion

The following is of the "Do what you need to do to make this work properly" variety:

Approach #5:

@AutoConfigureMetrics simple marker interface, nothing more
ContextCustomizer that sets new "global" 'management.metrics.export.enabled' based on whether the test is marked as @AutoConfigureMetrics
@ConditionalOnMetricExportsEnabled that inspects global and specific property to decide match or not

Status: works

Pros:
- allows all export to be disabled w/ single property but respects specific export overrides
- can be used outside of test-land
Cons:
- small change to existing auto-configs - but not functional breaking change
- users custom metric exporters will be respected in exclusion if they copy pattern and use the @ConditionalOnMetricExportsEnabled

Outcome

New integration test defaults is that only SimpleMeterRegistry will be enabled. The management.metrics.export[.<name>].enabled properties can be used to adjust that.

On Fri, Jul 17, 2020, 10:05 AM Stéphane Nicoll ***@***.***> wrote: I don't think anything in this merge request could affect that. Actually, it does indirectly as the actuator (and it's auto-configuration) is now added to the project. I don't know why that happens but on the other hand Actuator does not seem required. I'll investigate a bit more. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21658 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG4RTQ4344MCF6MXQAS3KSTR4BSDHANCNFSM4NRI6SMQ> .

onobc · 2020-07-19T23:30:11Z

@snicoll did you get a chance to follow up on the issue? If not, I can take a look in the next 24hrs.

snicoll · 2020-07-20T07:16:21Z

I've removed actuator as it's not required although I might change my mind and write a test with an actual registry implementation to validate its auto-configuration isn't enabled. I am on PTO the next 2 days and I've already started to polish things quite a bit in a branch. If you want to investigate how to prevent security to kick in the way it does, I am definitely interested. Thanks!

onobc · 2020-07-20T15:19:00Z

Enjoy your PTO! I will look into how to prevent security from kicking in as it will be good to know, regardless of final direction.

…

On Mon, Jul 20, 2020 at 2:16 AM Stéphane Nicoll ***@***.***> wrote: I've removed actuator as it's not required although I might change my mind and write a test with an actual registry implementation to validate its auto-configuration isn't enabled. I am on PTO the next 2 days and I've already started to polish things quite a bit in a branch. If you want to investigate how to prevent security to kick in the way it does, I am definitely interested. Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21658 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG4RTQYYBSRG5VVO6SWXGKLR4PVNNANCNFSM4NRI6SMQ> .

onobc · 2020-07-22T02:54:10Z

I just got around to look into this now.

Yes, you are correct, we do not need those dependencies in there. I believe I had them in there as I was thinking initially of a heavier integration test - however ended up just verifying the expected properties were set etc. I forgot to remove the deps once I switched gears on that.
The security is kicking in due to ManagementWebSecurityAutoConfiguration being available on classpath now. If you look at the RestDocsTestApplication used by the IT you will notice it was already excluding SecurityAutoConfiguration. The test goes green when we add the ManagementWebSecurityAutoConfiguration to the exclude list.

@SpringBootApplication(exclude = { SecurityAutoConfiguration.class, ManagementWebSecurityAutoConfiguration.class })

This commit introduces a new property to globally disable metrics export. In integration tests, this property is automatically set to disable everything but in-memory metrics. This commit also introduces a `@AutoConfigureMetrics` annotation that can be used for integration tests that require metrics export to operate as they would in an application. See spring-projectsgh-21658

See spring-projectsgh-21658

This commit introduces a new property to globally disable metrics export. In integration tests, this property is automatically set to disable everything but in-memory metrics. This commit also introduces a `@AutoConfigureMetrics` annotation that can be used for integration tests that require metrics export to operate as they would in an application. See gh-21658

See gh-21658

snicoll · 2020-07-24T13:46:34Z

@Bono007 thank you very much for making your first contribution to Spring Boot. I've polished your contribution in
3530ac9.

onobc · 2020-07-24T20:40:22Z

Ha! Its not my first contribution to Spring Boot. But thank you for the warm welcome ;)

wilkinsona · 2020-07-24T20:44:52Z

This is your third first contribution no less 😬. Sorry about that. IIRC, GitHub kept listing you as a first-time contributor for a while. That's no longer the case so hopefully this will be your last first contribution.

snicoll · 2020-07-25T05:13:48Z

I know something was off when I saw the badge but I decided to go with the warm welcome anyway. Thank you for making your third first contribution, indeed 😂

onobc · 2020-07-26T12:24:43Z

3rd times a charm - so they say. ;)

spring-projects-issues added the status: waiting-for-triage label Jun 3, 2020

onobc commented Jun 3, 2020

View reviewed changes

snicoll approved these changes Jul 16, 2020

View reviewed changes

snicoll added type: enhancement and removed status: waiting-for-triage labels Jul 16, 2020

snicoll added this to the 2.4.x milestone Jul 16, 2020

snicoll mentioned this pull request Jul 16, 2020

Add support for easily disabling metrics export #21616

Closed

onobc added 2 commits July 16, 2020 22:02

Add global metrics exporter disable property; set during tests with a…

ef6feea

…bility to override via @AutoConfigureMetrics. Closes gh-21616

Add auto-config test for each exporter and simplified javadoc.

8b462a2

snicoll self-assigned this Jul 17, 2020

snicoll changed the title ~~Add global metrics exporter disable property~~ Add support for easily disabling metrics export Jul 17, 2020

snicoll added a commit to snicoll/spring-boot that referenced this pull request Jul 24, 2020

Polish "Disable metrics export in integration tests"

52e090a

See spring-projectsgh-21658

snicoll changed the title ~~Add support for easily disabling metrics export~~ Disable metrics export in integration tests Jul 24, 2020

snicoll changed the title ~~Disable metrics export in integration tests~~ Add support for easily disabling metrics export Jul 24, 2020

snicoll modified the milestones: 2.4.x, 2.4.0-M2 Jul 24, 2020

snicoll added a commit that referenced this pull request Jul 24, 2020

Polish "Disable metrics export in integration tests"

3530ac9

See gh-21658

snicoll closed this in db462c4 Jul 24, 2020

VaniHaripriya mentioned this pull request Jan 17, 2022

[KOGITO-6497]-Certify Spring Boot 2.4.x apache/incubator-kie-kogito-runtimes#1838

Merged

plietar mentioned this pull request Sep 26, 2024

Expose metrics over HTTP. mrc-ide/packit#126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for easily disabling metrics export #21658

Add support for easily disabling metrics export #21658

onobc commented Jun 3, 2020

onobc Jun 3, 2020

onobc Jun 3, 2020

onobc commented Jun 3, 2020

snicoll left a comment

snicoll Jul 16, 2020

onobc commented Jul 16, 2020

onobc commented Jul 17, 2020

onobc commented Jul 17, 2020

snicoll commented Jul 17, 2020

snicoll commented Jul 17, 2020

onobc commented Jul 17, 2020 via email

onobc commented Jul 19, 2020

snicoll commented Jul 20, 2020 •

edited

Loading

onobc commented Jul 20, 2020 via email

onobc commented Jul 22, 2020 •

edited

Loading

snicoll commented Jul 24, 2020

onobc commented Jul 24, 2020

wilkinsona commented Jul 24, 2020 •

edited

Loading

snicoll commented Jul 25, 2020

onobc commented Jul 26, 2020

		* exporters. By default, all metrics exporters other than the in-memory
		* {@code SimpleMetricsExportAutoConfiguration} are disabled.

Add support for easily disabling metrics export #21658

Add support for easily disabling metrics export #21658

Conversation

onobc commented Jun 3, 2020

onobc Jun 3, 2020

Choose a reason for hiding this comment

onobc Jun 3, 2020

Choose a reason for hiding this comment

onobc commented Jun 3, 2020

Background

Goal

Approaches

Approach #1:

Approach #2:

Approach #3:

Approach #4:

Approach #5:

Outcome

Next

snicoll left a comment

Choose a reason for hiding this comment

snicoll Jul 16, 2020

Choose a reason for hiding this comment

onobc commented Jul 16, 2020

onobc commented Jul 17, 2020

onobc commented Jul 17, 2020

snicoll commented Jul 17, 2020

snicoll commented Jul 17, 2020

onobc commented Jul 17, 2020 via email

onobc commented Jul 19, 2020

snicoll commented Jul 20, 2020 • edited Loading

onobc commented Jul 20, 2020 via email

onobc commented Jul 22, 2020 • edited Loading

snicoll commented Jul 24, 2020

onobc commented Jul 24, 2020

wilkinsona commented Jul 24, 2020 • edited Loading

snicoll commented Jul 25, 2020

onobc commented Jul 26, 2020

snicoll commented Jul 20, 2020 •

edited

Loading

onobc commented Jul 22, 2020 •

edited

Loading

wilkinsona commented Jul 24, 2020 •

edited

Loading