Skip to content
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

Disable actuator http observations #36455

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonatan-ivanov
Copy link
Member

See: #34801

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 19, 2023
@jonatan-ivanov jonatan-ivanov force-pushed the disable-actuator-http-observations branch from cfb70fb to f37c5f0 Compare July 19, 2023 19:54
@@ -31,3 +31,11 @@ management.endpoints.migrate-legacy-ids=true

management.endpoints.jackson.isolated-object-mapper=true
spring.jackson.visibility.field=any

#management.tracing.sampling.probability=1.0
Copy link
Member Author

Choose a reason for hiding this comment

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

I left these changes in samples intentionally in the PR (it's a separate commit), one the PR is ready to merge I will drop it.

@mhalbritter mhalbritter added theme: observability Issues related to observability for: team-meeting An issue we'd like to discuss as a team to make progress labels Jul 26, 2023
@mhalbritter
Copy link
Contributor

I flagged this for team-meeting, as we need to discuss the following:

  • What's up with the WebClient tests? Is this a bug in the test or in the WebClient?
  • Should we test this in a different way?

@mhalbritter mhalbritter removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 2, 2023
@jonatan-ivanov jonatan-ivanov force-pushed the disable-actuator-http-observations branch 3 times, most recently from 6dd92ed to 9563ba7 Compare August 10, 2023 02:11
@jonatan-ivanov jonatan-ivanov force-pushed the disable-actuator-http-observations branch from 9563ba7 to f79d931 Compare August 10, 2023 17:33
@jonatan-ivanov jonatan-ivanov marked this pull request as ready for review August 10, 2023 17:55
Copy link
Contributor

@vpavic vpavic left a comment

Choose a reason for hiding this comment

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

I tried to add the code from ActuatorWebEndpointObservationConfiguration to one of my projects (Spring Boot 3.1.2 based) and I was greeted by a bean dependency cycle:

   webMvcObservationFilter defined in class path resource [org/springframework/boot/actuate/autoconfigure/observation/web/servlet/WebMvcObservationAutoConfiguration.class]
┌─────┐
|  observationRegistry defined in class path resource [org/springframework/boot/actuate/autoconfigure/observation/ObservationAutoConfiguration.class]
↑     ↓
|  actuatorWebEndpointObservationPredicate defined in class path resource [.../infrastructure/observation/ObservationConfiguration.class]
↑     ↓
|  pathMappedEndpoints defined in class path resource [org/springframework/boot/actuate/autoconfigure/endpoint/web/WebEndpointAutoConfiguration.class]
↑     ↓
|  quartzEndpoint defined in class path resource [org/springframework/boot/actuate/autoconfigure/quartz/QuartzEndpointAutoConfiguration.class]
↑     ↓
|  liquibase defined in class path resource [org/springframework/boot/autoconfigure/liquibase/LiquibaseAutoConfiguration$LiquibaseConfiguration.class]
└─────┘

Not sure if the same happens with the current main and related code being a part of auto-configuration but just a hint to look into it.

WebMvcProperties webMvcProperties, PathMappedEndpoints pathMappedEndpoints) {
return (name, context) -> {
if (context instanceof ServerRequestObservationContext serverContext) {
String endpointPath = getEndpointPath(serverProperties, webMvcProperties, pathMappedEndpoints);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since endpoint path doesn't change in runtime, I think it doesn't need to be resolved on each invocation of the predicate but rather only once when the predicate is set up.

PathMappedEndpoints pathMappedEndpoints) {
return (name, context) -> {
if (context instanceof ServerRequestObservationContext serverContext) {
String endpointPath = getEndpointPath(webFluxProperties, pathMappedEndpoints);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@cmergenthaler
Copy link

cmergenthaler commented Aug 31, 2023

Will that be part of the next 3.2.x release?

@mhalbritter
Copy link
Contributor

mhalbritter commented Aug 31, 2023

I doubt it. There are technical subtleties we need to solve before this lands in a Spring Boot release. But hey, never say never.

@mhalbritter
Copy link
Contributor

We're not happy with all the string concatenation and matching going on here. A better way to solve this would be to cancel the observation when an actuator endpoint has been hit. AFAIK there's no way to do that right now, so I've opened micrometer-metrics/micrometer#4061 to discuss this possibility with the Micrometer team. Let's see what they think about that idea.

@mhalbritter mhalbritter added type: enhancement A general enhancement status: blocked An issue that's blocked on an external project change and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 5, 2023
@mhalbritter mhalbritter added this to the 3.x milestone Sep 5, 2023
@maradanasai

This comment was marked as outdated.

@mhalbritter

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked An issue that's blocked on an external project change theme: observability Issues related to observability type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants