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

Closes gh-21616

Comment From: onobc

@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.

Next

I have submitted a handful of tests to sanity check the functionality and illustrate the usage of the new annotation. If this is a direction that we agree on then I will add the missing tests and update the docs accordingly in a subsequent commit.

Comment From: onobc

@snicoll I should have to add the tests and update the comments based on the feedback w/in the next 12 hours.

Comment From: onobc

@snicoll - I added the auto-config tests for each exporter and simplified the java doc.

Comment From: onobc

Is the failure around RestAssuredRestDocsAutoConfigurationAdvancedConfigurationIntegrationTests expected? Looks like its getting a 401 and when i hit the endpoint that its trying to hit I see a form based login screen appear. I don't think anything in this merge request could affect that.

Comment From: snicoll

Same. Don't worry about it, I'll investigate as part of polishing this change.

Comment From: snicoll

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.

Comment From: onobc

Ahh interesting. Sounds like security is kicking in now with the added deps.

On Fri, Jul 17, 2020, 10:05 AM Stéphane Nicoll notifications@github.com 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 https://github.com/spring-projects/spring-boot/pull/21658#issuecomment-660157974, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG4RTQ4344MCF6MXQAS3KSTR4BSDHANCNFSM4NRI6SMQ .

Comment From: onobc

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

Comment From: snicoll

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!

Comment From: onobc

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 notifications@github.com 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 https://github.com/spring-projects/spring-boot/pull/21658#issuecomment-660849605, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG4RTQYYBSRG5VVO6SWXGKLR4PVNNANCNFSM4NRI6SMQ .

Comment From: onobc

I just got around to look into this now.

  1. 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.

  2. 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 })

Comment From: snicoll

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

Comment From: onobc

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

Comment From: wilkinsona

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.

Comment From: snicoll

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 😂

Comment From: onobc

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