Skip to content

Add support for the @SpanTag annotation #38662

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

Closed
lkleeven opened this issue Dec 5, 2023 · 7 comments
Closed

Add support for the @SpanTag annotation #38662

lkleeven opened this issue Dec 5, 2023 · 7 comments
Labels
type: enhancement A general enhancement
Milestone

Comments

@lkleeven
Copy link

lkleeven commented Dec 5, 2023

Spring Boot 3.2.0 came with a number of Observability improvements, supporting a number of Micrometer annotations.

@SpanTag is not mentioned here, and according to our tests, also not working out of the box. Is this done by design or would this be a bug?

To make things easier, I've made a demo project with a unit test that reproduces the issue. You can do so by opening the project and running the AopTracingTest or running ./mvnw clean verify. Next to the @SpanTag annotation it also verifies the working of other tracing annotations, which currently do work.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 5, 2023
@bclozel
Copy link
Member

bclozel commented Dec 5, 2023

Looking at #37640, it seems that the application needs to define a custom SpanTagAnnotationHandler bean. @jonatan-ivanov can you explain why this is needed? Should we improve the documentation to mention our support for those annotations?

@lkleeven
Copy link
Author

lkleeven commented Dec 5, 2023

Maybe good to mention. While using Spring Boot 3.1.0 we provided this for the users of our framework with the following autoconfiguration where we did not need to define any SpanTagAnnotationHandler, also not anywhere else:

/**
 * Creates the beans needed to be able to use AOP for the {@link io.micrometer.tracing.annotation.NewSpan},
 * {@link io.micrometer.tracing.annotation.ContinueSpan} and {@link io.micrometer.tracing.annotation.SpanTag} annotations
 */
@AutoConfiguration(after = AxleOpenTelemetryTracingAutoConfiguration.class)
@ConditionalOnEnabledTracing
@ConditionalOnProperty(value = AxleTracingProperties.AopProperties.PROP_ENABLED, havingValue = "true", matchIfMissing = true)
public class AxleTracingAopAutoConfiguration {

    @Bean
    @ConditionalOnMissingBean
    public NewSpanParser axleNewSpanParser() {
        return new DefaultNewSpanParser();
    }

    @Bean
    @ConditionalOnMissingBean
    public MethodInvocationProcessor axleMethodInvocationProcessor(
            NewSpanParser newSpanParser,
            Tracer tracer,
            BeanFactory beanFactory
    ) {
        return new ImperativeMethodInvocationProcessor(
                newSpanParser,
                tracer,
                beanFactory::getBean,
                beanFactory::getBean
        );
    }

    @Bean
    @ConditionalOnMissingBean
    public SpanAspect axleSpanAspect(MethodInvocationProcessor methodInvocationProcessor) {
        return new SpanAspect(methodInvocationProcessor);
    }
}

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Dec 5, 2023

SpanTagAnnotationHandler should be able to resolve the key and the value in some way from the @SpanTag annotation and set them on the span but right now we did not add an opinion about what that behavior should be. There are multiple ways to do this and different users might have different needs. We can add a default behavior I'm not sure though what should it be. Fyi: Micrometer Tracing has an example that supports SpEL: https://micrometer.io/docs/tracing#_aspect_oriented_programming_starting_from_micrometer_tracing_1_1_0

Btw there is also @MeterTag which right now only works for @Timed.

@marcingrzejszczak
Copy link
Contributor

In Spring Cloud Sleuth the default was SPel. Micrometer Tracing has no notion of SPel nor Spring so it can't ship this component. That should be coming from Boot I guess? 🤷

@mhalbritter
Copy link
Contributor

I have something in this branch. It auto-configures the SpanTagAnnotationHandler and implements a SpEL based ValueExpressionResolver.

@marcingrzejszczak
Copy link
Contributor

I checked it out and it makes perfect sense @mhalbritter 👍

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Dec 7, 2023
@mhalbritter mhalbritter changed the title Is there a specific reason why @SpanTag is not supported in Spring Boot 3.2.0 Add support for the @SpanTag annotation Dec 12, 2023
@mhalbritter mhalbritter added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Dec 12, 2023
@mhalbritter mhalbritter added this to the 3.3.x milestone Dec 12, 2023
@mhalbritter
Copy link
Contributor

mhalbritter commented Dec 12, 2023

We decided to ship that as an enhancement in 3.3. If you want to have it for 3.2 already, please see the code in my branch. It's not that much:

@Bean
@ConditionalOnMissingBean
SpanTagAnnotationHandler spanTagAnnotationHandler(BeanFactory beanFactory) {
  ValueExpressionResolver valueExpressionResolver = new SpelTagValueExpressionResolver();
  return new SpanTagAnnotationHandler(beanFactory::getBean, (ignored) -> valueExpressionResolver);
}
private static class SpelTagValueExpressionResolver implements ValueExpressionResolver {

  @Override
  public String resolve(String expression, Object parameter) {
    try {
      SimpleEvaluationContext context = SimpleEvaluationContext.forReadOnlyDataBinding().build();
      ExpressionParser expressionParser = new SpelExpressionParser();
      Expression expressionToEvaluate = expressionParser.parseExpression(expression);
      return expressionToEvaluate.getValue(context, parameter, String.class);
    } catch (Exception ex) {
      throw new IllegalStateException("Unable to evaluate SpEL expression '%s'".formatted(expression), ex);
    }
  }
}

@mhalbritter mhalbritter modified the milestones: 3.3.x, 3.3.0-M1 Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants