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

Auto-configuration ignores user-provided ObservationConventions #33285

Closed
braunsonm opened this issue Nov 21, 2022 · 7 comments
Closed

Auto-configuration ignores user-provided ObservationConventions #33285

braunsonm opened this issue Nov 21, 2022 · 7 comments
Assignees
Labels
theme: observability Issues related to observability type: bug A general bug
Milestone

Comments

@braunsonm
Copy link

braunsonm commented Nov 21, 2022

With the move to micrometer observability, the WebClientExchangeTagsProvider and RestTemplateExchangeTagsProvicer have both been deprecated in favour of ObservationConventions.

The configuration (in this case the reactive but also affects the servlet) is not customizable enough to allow users to move off the deprecated classes.

The configuration should have means to accept a custom implementation of the ObservationConvention. Right now you would have to exclude this configuration class completely as the default convention is created in the bean method. See here:
https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/observation/web/client/WebClientObservationConfiguration.java#L53

Additional Context

I would like to migrate away from deprecated classes, but without being able to create my own ObservationConvention I cannot cleanly stop using the old TagsProvider's.

cc @bclozel excuse the tag, but mentioned as you created these configuration classes

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 21, 2022
@wilkinsona
Copy link
Member

Brian's been working on a migration guide for this. Does https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-Metrics-3.0#migrating-tag-providers-and-contributors help?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Nov 21, 2022
@bclozel
Copy link
Member

bclozel commented Nov 21, 2022

@braunsonm no worries.

Does this section of the migration guide helps? Could you tell us more about your use case and how we could improve it?

@braunsonm
Copy link
Author

That does help, I was unaware that multiple implementations to the ObservationRegistry effectively override the ones provided by Spring Boot.
Thank you, this satisfies my use case.

@bclozel
Copy link
Member

bclozel commented Nov 21, 2022

Thanks!

@bclozel bclozel added status: invalid An issue that we don't feel is valid and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Nov 21, 2022
@bclozel
Copy link
Member

bclozel commented Nov 21, 2022

We've also addressed this in the reference documentation in #33281

@braunsonm
Copy link
Author

braunsonm commented Nov 21, 2022

Hey @bclozel We are not seeing it behave the way you describe in the documentation. Would you like a separate issue for this?

With a very simple example:

public class ExtendedClientRequestObservationConvention extends DefaultClientRequestObservationConvention implements
  GlobalObservationConvention<ClientRequestObservationContext> {
  @Override
  public KeyValues getLowCardinalityKeyValues(ClientRequestObservationContext context) {
    // Note the URI has been removed
    return KeyValues.of(method(context), status(context), exception(context), outcome(context));
  }
}
@Configuration
public class SampleConfiguration {
  @Bean
  ExtendedClientRequestObservationConvention sampleConvention() {
    return new ExtendedClientRequestObservationConvention();
  }
}

I can still see that the Default implementation is called, even when the bean is wired up.

@braunsonm braunsonm reopened this Nov 21, 2022
@bclozel bclozel added type: bug A general bug and removed status: invalid An issue that we don't feel is valid labels Nov 22, 2022
@bclozel bclozel self-assigned this Nov 22, 2022
@bclozel bclozel added this to the 3.0.0 milestone Nov 22, 2022
@bclozel bclozel added the theme: observability Issues related to observability label Nov 22, 2022
@bclozel bclozel changed the title WebClientObservationConfiguration is not sufficiently customizable Leverage user-provided ObservationConventions in auto-configurations Nov 22, 2022
@wilkinsona wilkinsona changed the title Leverage user-provided ObservationConventions in auto-configurations Auto-configuration ignores user-provided ObservationConventions Nov 22, 2022
@bclozel
Copy link
Member

bclozel commented Nov 22, 2022

Thanks @braunsonm - we've discussed this with @marcingrzejszczak and found that the current situation is not in line with the migration story we're expecting.
I'm repurposing this issue to use ObservationConvention beans provided by the application as a way to customize key values. I've also updated the migration guide accordingly. You can test this approach with 3.0.0-SNAPSHOT as soon as this issue is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: observability Issues related to observability type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants