-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add ObservationRegistry support to ConfigClientRequestTemplateFactory #3221
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
5ce43bd
4cecc0c
4eb48ee
8097f33
f4e5627
b41d108
01209b2
96a9967
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -240,8 +240,14 @@ public List<ConfigServerConfigDataResource> resolveProfileSpecific( | |
| .registerSingleton("configDataConfigClientProperties", | ||
| event.getBootstrapContext().get(ConfigClientProperties.class))); | ||
|
|
||
| bootstrapContext.registerIfAbsent(ConfigClientRequestTemplateFactory.class, | ||
| context -> new ConfigClientRequestTemplateFactory(log, context.get(ConfigClientProperties.class))); | ||
| bootstrapContext.registerIfAbsent(ConfigClientRequestTemplateFactory.class, context -> { | ||
| ConfigClientProperties props = context.get(ConfigClientProperties.class); | ||
| if (ClassUtils | ||
| .isPresent("org.springframework.boot.restclient.observation.ObservationRestTemplateCustomizer", null)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point we are too early in the lifecycle here to check the presence of the bean instead of the class, right?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's correct. At this point we are in the bootstrap phase, so the ApplicationContext |
||
| return ObservationConfigClientRequestTemplateFactory.createWithObservation(context, log, props); | ||
| } | ||
| return new ConfigClientRequestTemplateFactory(log, props); | ||
| }); | ||
|
|
||
| bootstrapContext.registerIfAbsent(RestTemplate.class, context -> { | ||
| ConfigClientRequestTemplateFactory factory = context.get(ConfigClientRequestTemplateFactory.class); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /* | ||
| * Copyright 2026-present the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.cloud.config.client; | ||
|
|
||
| import io.micrometer.observation.ObservationRegistry; | ||
| import org.apache.commons.logging.Log; | ||
|
|
||
| import org.springframework.boot.bootstrap.BootstrapContext; | ||
| import org.springframework.boot.restclient.observation.ObservationRestTemplateCustomizer; | ||
| import org.springframework.http.client.observation.DefaultClientRequestObservationConvention; | ||
| import org.springframework.web.client.RestTemplate; | ||
|
|
||
| public class ObservationConfigClientRequestTemplateFactory extends ConfigClientRequestTemplateFactory { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be public (as well as its ctor)? |
||
|
|
||
| private final ObservationRestTemplateCustomizer observationRestTemplateCustomizer; | ||
|
|
||
| public ObservationConfigClientRequestTemplateFactory(Log log, ConfigClientProperties properties, | ||
| ObservationRegistry observationRegistry) { | ||
| super(log, properties); | ||
| this.observationRestTemplateCustomizer = observationRegistry != ObservationRegistry.NOOP | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use |
||
| ? new ObservationRestTemplateCustomizer(observationRegistry, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we/should we reuse the bean that Boot creates (see I mean if the user disables RestTemplateObservationAutoConfiguration, should the observation config for this
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we use the addCloseListener approach, this would be partially handled — if the user However, the RestTemplate would still be instrumented during the bootstrap phase since To fully respect the user's intent, we would need to check during bootstrap whether the Does this approach make sense, or is there a better way to handle this? |
||
| new DefaultClientRequestObservationConvention()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one reason why we should not create a new
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. Using a hardcoded DefaultClientRequestObservationConvention prevents We thought about an approach to solve both issues — the convention and the registry sync: The limitation of this approach is that observations from the initial bootstrap call Given that the issue mentions retries as a key use case:
|
||
| : null; | ||
| } | ||
|
|
||
| @Override | ||
| public RestTemplate create() { | ||
| RestTemplate template = super.create(); | ||
| if (observationRestTemplateCustomizer != null) { | ||
| observationRestTemplateCustomizer.customize(template); | ||
| } | ||
| return template; | ||
| } | ||
|
|
||
| static ConfigClientRequestTemplateFactory createWithObservation(BootstrapContext context, Log log, | ||
| ConfigClientProperties props) { | ||
| ObservationRegistry registry = context.getOrElse(ObservationRegistry.class, ObservationRegistry.NOOP); | ||
| return new ObservationConfigClientRequestTemplateFactory(log, props, registry); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * Copyright 2026-present the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.cloud.config.client; | ||
|
|
||
| import io.micrometer.observation.ObservationRegistry; | ||
|
|
||
| import org.springframework.boot.bootstrap.BootstrapRegistry; | ||
|
|
||
| public class ObservationConfigServerBootstrapper extends ConfigServerBootstrapper { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be public (and its ctor)? |
||
|
|
||
| private ObservationRegistry observationRegistry; | ||
|
|
||
| public static ObservationConfigServerBootstrapper create() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed (we already have a ctor with the same empty params)? |
||
| return new ObservationConfigServerBootstrapper(); | ||
| } | ||
|
|
||
| public ObservationConfigServerBootstrapper withObservationRegistry(ObservationRegistry observationRegistry) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to enable this by default without the user need to do this? SpringApplication app = new SpringApplication(MyApp.class);
app.addBootstrapRegistryInitializer(
new ConfigServerBootstrapper()
.withObservationRegistry(myObservationRegistry)
);
app.run(args);Also, I have two questions in connection with this:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, the PR description should use new ObservationConfigServerBootstrapper() Regarding enabling this by default without the user needing to configure it manually — Regarding where users should get the ObservationRegistry — this is exactly the problem
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You mean this PR should solve it or you have an idea to solve this? (I'm asking because I think the current changes PR do not solve the issue.)
I don't know, can you tell me more about the the addCloseListener approach or can you share some code I can look at?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea is to use addCloseListener from the BootstrapContext to re-instrument the RestTemplate Here is a rough idea of how this would work: bootstrapContext.addCloseListener(event -> {
ApplicationContext applicationContext = event.getApplicationContext();
if (applicationContext.containsBean(ObservationRestTemplateCustomizer.class.getName())) {
ObservationRestTemplateCustomizer customizer = applicationContext
.getBean(ObservationRestTemplateCustomizer.class);
RestTemplate restTemplate = event.getBootstrapContext().get(RestTemplate.class);
customizer.customize(restTemplate);
}
});The limitation of this approach is that the initial bootstrap call to the Config Server Does this approach make sense? Is there a better way to handle this?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess that's ok, maybe we can add more logs (in a different issue/PR) if needed to help people troubleshooting things.
I think it makes sense. Or let me say I can imagine a situation where this would work but the last time I needed to interact with the bootstrap context was many years ago. :) Have you tried this? @ryanjbaxter What do you think? |
||
| this.observationRegistry = observationRegistry; | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public void initialize(BootstrapRegistry registry) { | ||
| super.initialize(registry); | ||
| if (observationRegistry != null) { | ||
| registry.register(ObservationRegistry.class, BootstrapRegistry.InstanceSupplier.of(observationRegistry)); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /* | ||
| * Copyright 2026-present the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.cloud.config.client; | ||
|
|
||
| import io.micrometer.observation.ObservationRegistry; | ||
| import org.apache.commons.logging.LogFactory; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import org.springframework.web.client.RestTemplate; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * @author Luccas Asaphe | ||
| * | ||
| */ | ||
| public class ConfigClientRequestTemplateFactoryTests { | ||
|
|
||
| @Test | ||
| void shouldInstrumentRestTemplateWhenObservationRegistryProvided() { | ||
|
LuccasAps marked this conversation as resolved.
Outdated
|
||
| // 1. set up the factory | ||
| ConfigClientProperties properties = new ConfigClientProperties(); | ||
| ObservationRegistry registry = ObservationRegistry.create(); | ||
| ObservationConfigClientRequestTemplateFactory factory = new ObservationConfigClientRequestTemplateFactory( | ||
| LogFactory.getLog(getClass()), properties, registry); | ||
|
|
||
| // 2. create the RestTemplate | ||
| RestTemplate restTemplate = factory.create(); | ||
|
|
||
| // 3. verify the observation registry | ||
| assertThat(restTemplate.getObservationRegistry()).isEqualTo(registry); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be much better to verify if
|
||
| } | ||
|
|
||
| @Test | ||
| void shouldNotInstrumentRestTemplateWhenObservationRegistryNotProvided() { | ||
| // 1. set up the factory | ||
| ConfigClientProperties properties = new ConfigClientProperties(); | ||
| ConfigClientRequestTemplateFactory factory = new ConfigClientRequestTemplateFactory( | ||
| LogFactory.getLog(getClass()), properties); | ||
|
|
||
| // 2. create the RestTemplate | ||
| RestTemplate restTemplate = factory.create(); | ||
|
|
||
| // 3. verify the observation registry | ||
| assertThat(restTemplate.getObservationRegistry()).isEqualTo(ObservationRegistry.NOOP); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldNotInstrumentRestTemplateWhenObservationRegistryIsNoop() { | ||
| // 1. set up the factory | ||
| ConfigClientProperties properties = new ConfigClientProperties(); | ||
| ObservationRegistry registry = ObservationRegistry.NOOP; | ||
|
LuccasAps marked this conversation as resolved.
Outdated
|
||
| ObservationConfigClientRequestTemplateFactory factory = new ObservationConfigClientRequestTemplateFactory( | ||
| LogFactory.getLog(getClass()), properties, registry); | ||
|
|
||
| // 2. create the RestTemplate | ||
| RestTemplate restTemplate = factory.create(); | ||
|
|
||
| // 3. verify the observation registry | ||
| assertThat(restTemplate.getObservationRegistry()).isEqualTo(ObservationRegistry.NOOP); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /* | ||
| * Copyright 2026-present the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.cloud.config.client; | ||
|
|
||
| import io.micrometer.observation.ObservationRegistry; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; | ||
| import org.springframework.boot.SpringBootConfiguration; | ||
| import org.springframework.boot.WebApplicationType; | ||
| import org.springframework.boot.autoconfigure.EnableAutoConfiguration; | ||
| import org.springframework.boot.bootstrap.BootstrapContext; | ||
| import org.springframework.boot.builder.SpringApplicationBuilder; | ||
| import org.springframework.cloud.test.ClassPathExclusions; | ||
| import org.springframework.context.ConfigurableApplicationContext; | ||
| import org.springframework.web.client.RestTemplate; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * @author Luccas Asaphe | ||
| * | ||
| */ | ||
| @ClassPathExclusions({ "spring-boot-restclient-*.jar" }) | ||
| public class ConfigServerConfigDataWithoutMicrometerTests { | ||
|
|
||
| @Test | ||
| void contextStartsWithoutMicrometer() { | ||
|
LuccasAps marked this conversation as resolved.
|
||
| try (ConfigurableApplicationContext context = new SpringApplicationBuilder(TestConfig.class) | ||
| .web(WebApplicationType.NONE) | ||
| .addBootstrapRegistryInitializer(registry -> registry.addCloseListener(event -> { | ||
| BootstrapContext bootstrapContext = event.getBootstrapContext(); | ||
| ConfigurableListableBeanFactory beanFactory = event.getApplicationContext().getBeanFactory(); | ||
|
|
||
| RestTemplate restTemplate = bootstrapContext.get(RestTemplate.class); | ||
| beanFactory.registerSingleton("restTemplate", restTemplate); | ||
| })) | ||
| .run("--spring.config.import=optional:configserver:")) { | ||
| assertThat(context).isNotNull(); | ||
| assertThat(context.getBean(RestTemplate.class).getObservationRegistry()) | ||
| .isEqualTo(ObservationRegistry.NOOP); | ||
| } | ||
| } | ||
|
|
||
| @SpringBootConfiguration | ||
| @EnableAutoConfiguration | ||
| static class TestConfig { | ||
|
|
||
| } | ||
|
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Why does this need to be public?
It seems the only new place where this is used is
ObservationConfigServerBootstrapperwhich is in the same package and it might be package-private as well asConfigServerBootstrapper.