Skip to content

Add tests to verify that every config setting for each auto-configured meter registry is exposed as a configuration property#33743

Closed
msobeck wants to merge 4 commits intospring-projects:mainfrom
msobeck:pr/30901
Closed

Add tests to verify that every config setting for each auto-configured meter registry is exposed as a configuration property#33743
msobeck wants to merge 4 commits intospring-projects:mainfrom
msobeck:pr/30901

Conversation

@msobeck
Copy link
Copy Markdown
Contributor

@msobeck msobeck commented Jan 10, 2023 •
edited by philwebb
Loading

Closes #30901.

I added the missing PropertiesConfigAdapterTests and enhanced them, so that newly added
fields to micrometer configs, that are not settable via properties, will let the test fail. (if you don't specifically exclude them)

Not overridden default methods in adapters are the most common cause of forgotten field exposure in this scenario, because they do not for force an override in the adapter. (see #30898)

TestConfigsToPropertiesExposure will check for not overridden config default methods in the adapter class.
This can be an indicator for micrometer config fields, that have been forgotten to expose via properties.
They already have found some not exposed config fields f.e. for AtlasConfig:

@Test
void allConfigDefaultMethodsAreOverriddenByAdapter() {
	assertThatAllConfigDefaultMethodsAreOverriddenByAdapter(AtlasConfig.class, AtlasPropertiesConfigAdapter.class,
			"lwcIgnorePublishStep",
			"initialPollingDelay",
			"autoStart",
			"lwcStep",
			"validTagCharacters");
}

I declared them as excluded, because i don't know if they are intentionally not exposed.
Someone could look at this in another task. This PR only shows the current state.
I am aware that these tests are just a rough help against human forgetfulness and can be reverted if they don't fit in.

@pivotal-cla
Copy link
Copy Markdown

pivotal-cla commented Jan 10, 2023

@msobeck Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@msobeck msobeck changed the title Pr/30901 Add tests to verify that every config setting for each auto-configured meter registry is exposed as a configuration property Jan 10, 2023
@pivotal-cla
Copy link
Copy Markdown

pivotal-cla commented Jan 10, 2023

@msobeck Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 10, 2023
@mhalbritter mhalbritter mentioned this pull request Jan 31, 2023
Closed
@mhalbritter mhalbritter added status: waiting-for-triage An issue we've not yet triaged and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 31, 2023
@msobeck msobeck force-pushed the pr/30901 branch 2 times, most recently from 574a23a to 78b8d0d Compare February 8, 2023 16:36
@wilkinsona wilkinsona self-assigned this Feb 15, 2023
This was referenced Feb 15, 2023
Closed
Closed
@wilkinsona wilkinsona reopened this Feb 15, 2023
@msobeck
Copy link
Copy Markdown
Contributor Author

msobeck commented Feb 21, 2023 •
edited
Loading

@wilkinsona thanks for the review! Is there anything left in this PR? Otherwise I would suggest to close this one, since the missing tests were added in ff04f00.

wilkinsona pushed a commit to wilkinsona/spring-boot that referenced this pull request Feb 21, 2023
@msobeck @wilkinsona
Test Micrometer config to property exposure
b7bc06a 
See spring-projectsgh-33743
wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request Feb 21, 2023
@wilkinsona
Polish "Test Micrometer config to property exposure"
7057db8 
See spring-projectsgh-33743
@wilkinsona
Copy link
Copy Markdown
Member

wilkinsona commented Feb 21, 2023 •
edited
Loading

Yeah, there's quite a bit left. ff04f00 didn't include any of the changes to verify that each config setting has a property. That's what we'll merge in this PR. I'm in the process of polishing those changes: https://github.com/wilkinsona/spring-boot/tree/gh-33743.

@wilkinsona wilkinsona added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 21, 2023
@wilkinsona wilkinsona added this to the 2.7.x milestone Feb 21, 2023
jonatan-ivanov
jonatan-ivanov requested changes Feb 21, 2023
View reviewed changes
...ringframework/boot/actuate/autoconfigure/metrics/export/TestConfigsToPropertiesExposure.java

}

private static final List<Class<?>> classesForPropertiesList = List.of(Boolean.class, Byte.class, Character.class,
Copy link
Copy Markdown
Member

@jonatan-ivanov jonatan-ivanov Feb 21, 2023

Choose a reason for hiding this comment

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

Can we add Enum too?
We have quite a few use-cases, e.g.: HistogramFlavor for PrometheusConfig or DynatraceApiVersion for DynatraceConfig

Copy link
Copy Markdown
Member

@wilkinsona wilkinsona Feb 22, 2023

Choose a reason for hiding this comment

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

Thanks for taking a look, @jonatan-ivanov. I've removed the allow-list approach in my polishing. We now consider all default methods with no parameters, except those that are deprecated or that return Validated. The code that does that is here.

@jonatan-ivanov
Copy link
Copy Markdown
Member

jonatan-ivanov commented Feb 21, 2023

I really like the idea of having verifications for this, thank you!

@wilkinsona wilkinsona mentioned this pull request Jul 21, 2023
Closed
This was referenced Jul 21, 2023
Closed
Closed
Closed
wilkinsona pushed a commit to wilkinsona/spring-boot that referenced this pull request Jul 24, 2023
@msobeck @wilkinsona
Test Micrometer config to property exposure
be1f1c8 
See spring-projectsgh-33743
wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request Jul 24, 2023
@wilkinsona
Polish "Test Micrometer config to property exposure"
c2b43e7 
See spring-projectsgh-33743
wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request Jul 24, 2023
@wilkinsona
Merge pull request spring-projects#33743 from msobeck
7955e16 
* spring-projectsgh-33743:
  Polish "Test Micrometer config to property exposure"
  Test Micrometer config to property exposure

Closes spring-projectsgh-33743
@wilkinsona wilkinsona mentioned this pull request Jul 25, 2023
Closed
@wilkinsona wilkinsona modified the milestones: 2.7.x, 2.7.15 Jul 25, 2023
wilkinsona pushed a commit that referenced this pull request Jul 25, 2023
@msobeck @wilkinsona
Test Micrometer config to property exposure
19e7005 
See gh-33743
wilkinsona added a commit that referenced this pull request Jul 25, 2023
@wilkinsona
Polish "Test Micrometer config to property exposure"
5c6a912 
See gh-33743
@wilkinsona
Copy link
Copy Markdown
Member

wilkinsona commented Jul 25, 2023

Thanks very much, @msobeck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wilkinsona wilkinsona wilkinsona left review comments

@jonatan-ivanov jonatan-ivanov jonatan-ivanov requested changes

Assignees

@wilkinsona wilkinsona

Labels

type: task A general task

Projects

None yet

Milestone

2.7.15

Development

Successfully merging this pull request may close these issues.

Add tests to verify that every config setting for each auto-configured meter registry is exposed as a configuration property

6 participants

@msobeck @pivotal-cla @wilkinsona @jonatan-ivanov @spring-projects-issues @mhalbritter