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

Wavefront MeterRegistryCustomizer is not applying application tags from application.properties #33244

Closed
philwebb opened this issue Nov 17, 2022 · 12 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@philwebb
Copy link
Member

See #32844

The MeterRegistryCustomizer isn't getting created when the dependent ApplicationTags bean is instantiated from a properties file.

It works if an ApplicationTags bean is created via code.

This is probably an ordering issue, where WavefrontTracingAutoConfiguration runs after WavefrontMetricsAutoConfiguration.

@philwebb philwebb added the type: bug A general bug label Nov 17, 2022
@philwebb philwebb added this to the 3.0.x milestone Nov 17, 2022
@oppegard
Copy link
Contributor

I forked the Observability for Boot 3.0 blog post to reproduce this issue: https://github.com/oppegard/observability-boot-blog-post. Both the client and server application.properties are populated with management.wavefront.tracing values to instantiate an ApplicationTags bean from WavefrontTracingAutoConfiguration.

If you check the metric my.observation from the Client app, or jvm.gc.overhead from both Client and Server, they don't have the tags application, service, cluster, or shard.

If an ApplicationTags bean is created via code in ClientApplication, then the tags do show on metrics emited from its WavefrontMeterRegistry.

@philwebb philwebb self-assigned this Nov 17, 2022
@philwebb
Copy link
Member Author

This issue actually highlighted that ApplicationTags are now used for both metrics and tracing. As such, they don't make much sense under management.wavefront.tracing so I've moved them up one level.

@philwebb philwebb modified the milestones: 3.0.x, 3.0.0 Nov 18, 2022
@oppegard
Copy link
Contributor

@philwebb ApplicationTags should only be instantiated if WF distributed tracing is enabled (by including the micrometer-tracing-reporter-wavefront dependency). WF distributed tracing has tags to identify a microservice within a broader application: application, service, cluster, shard. We only want those tags to be included on metrics if DT is enabled.

The fix pushed at fb5cdbd has the unintended consequence of always instantiating an ApplicationTags bean and including them on metrics, even when micrometer-tracing-reporter-wavefront is not included.

@oppegard
Copy link
Contributor

Also, I think it makes more sense for the properties to live on management.wavefront.tracing since it applies only when DT is enabled (even though it affects metrics and spans – yeah, kinda confusing). I can go either way on that though.

@wilkinsona
Copy link
Member

That worries me a bit from an upgrade perspective, although I'm not sufficiently knowledgable about Wavefront to know what the impact may be.

If a user's already using Wavefront for metrics and they want to start using it for tracing as well, what would be the impact of the tags changing on their metrics just because they've started to use tracing? Could it break existing dashboards?

@philwebb
Copy link
Member Author

When I was searching for usages I found one in WavefrontTracingAutoConfiguration.wavefrontSpanHandler and one in WavefrontMetricsExportAutoConfiguration.wavefrontApplicationTagsCustomizer. That made me think that the tags were common to metrics and tracing.

It does feel a little odd that setting up ApplicationTags for tracing has an impact on metrics.

@oppegard
Copy link
Contributor

We've viewed the ApplicationTags as being additional tags on metrics. However, if their existing metrics have tags of application, service, cluster, or shard, then there will be a conflict when they start using distributed tracing (for DT to work, the ApplicationTags bean needs to take precence). This is the current behavior with wavefront-spring-boot for Boot 2.x apps.

We have existing out-of-the-box dashboards in Wavefront that will only function when spans and metrics having matching ApplicationTags. So if a user were to adopt distributed tracing down the line, they need to adapt their existing dashboard or make ApplicationTags conform to their existing values.

The tags are used for things like displaying JVM info for a service, as well as RED metrics: https://docs.wavefront.com/tracing_basics.html#examine-application-red-metrics-using-service-dashboard

@philwebb
Copy link
Member Author

Let me see if I can summarize to make sure we understand things correctly:

With wavefront-spring-boot a user could define ApplicationTags either as an @Bean or using wavefront.application.* properties. (at least that's my understanding from reading https://docs.wavefront.com/wavefront_springboot.html).

When the ApplicationTags bean exists, it's adapted to micrometer metrics tags. This is done in WavefrontMetricsConfiguration.

It looks like tags are also required if JVM metrics are being exported. They are also needed for the WavefrontTracer

It looks to me like wavefront-spring-boot also always creates an ApplicationTags bean. This is done in WavefrontAutoConfiguration. There's also a ApplicationTagsBuilderCustomizer interface that can be used if the user wants to customize the tags before they are built.

It feels like Spring Boot 3.0 is doing something pretty similar. We're creating the ApplicationTags bean from properties if it doesn't exist and applying it to metrics and tracing.

I wonder if we should move the properties we've added under management.wavefront.application. That way, someone migrating from wavefront-spring-boot could rename their wavefront.application properties?

AFAICT the ApplicationTags bean should always be created.

@philwebb philwebb reopened this Nov 18, 2022
@oppegard
Copy link
Contributor

oppegard commented Nov 18, 2022

I was mistaken about wavefront-spring-boot not always creating an ApplicationTags bean. I'd argue this is a bug, but was overlooked since the project was created with users of distributed tracing as the primary persona. It was likely considered an outlier for a user to disable distributed tracing (Stephane Nicoll might remember, since it looks like he was the original author).

Existing metrics-only users were already enabled via Actuator, and no ApplicationTags are included: https://docs.spring.io/spring-boot/docs/2.7.5/reference/html/actuator.html#actuator.metrics.export.wavefront.

I still feel that a default ApplicationTags bean shouldn't be created if a user doesn't have distributed tracing enabled, since that class/bean was created with the specific intent of enabling DT-related features.

Metrics-only users will likely be annoyed with default tags of application=unnamed_application and service=<spring.application.name>, and no way to easily exclude them (maybe Spring has an easy way to exclude, I'm not sure).

@philwebb
Copy link
Member Author

What's the reason that ApplicationTags get applied to MeterRegistry beans? If they're just for distributed tracing, do we really need them on the metrics?

@oppegard
Copy link
Contributor

@philwebb you can close this issue.

After taking a close look at Wavefront's Spring Boot dashboard and its source code, it only works if the required ApplicationTags of "application" and "service" are included on metrics. We're good to go with what you commited yesterday: fb5cdbd. Sorry for the confusion!

@philwebb
Copy link
Member Author

Oh cool. Actually, I might refine the properties and move them under management.wavefront.application before we close it again.

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

No branches or pull requests

3 participants