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

OtlpMetricsExportAutoConfiguration uses default OTLP metrics url when value comes from Dynamic Properties #41276

Closed
eddumelendez opened this issue Jun 30, 2024 · 3 comments
Labels
status: duplicate A duplicate of another issue type: bug A general bug

Comments

@eddumelendez
Copy link
Contributor

I am using grafana/otel-lgtm:0.6.0 image in order to test OpenTelemetry support (metrics, traces, logging) in Spring Boot 3.4.0-SNAPSHOT and it can not be used with @ServiceConnection("otel/opentelemetry-collector-contrib") because there is no Docker Compose/Testcontainers implementation for OtlpLoggingConnectionDetails. So, the values are contributed via Dynamic Properties as you can see in the reproduces linked below. Using @ServiceConnection("otel/opentelemetry-collector-contrib"), metrics and tracing works as expected.

Reproducer: https://github.com/eddumelendez/spring-boot-testcontainers-grafana-lgtm/tree/otlp_logs

./mvnw spring-boot:test-run
http :8080/actuator/configprops

Values for Tracing and Logging comes from Testcontainers but metrics was not contributed.

I tried to dig into it but I am missing something about the Export Metrics AutoConfiguration configuration order.

Also, next steps:

  • Support grafana/otel-lgtm image for Service Connection. I think Testcontainers should provide a LgtmStackContainer as part of a brand new grafana module
  • Add ConnectionDetails support for OtlpLoggingConnectionDetails (Docker Compose/Testcontainers)
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 30, 2024
@wilkinsona wilkinsona self-assigned this Jul 1, 2024
@wilkinsona
Copy link
Member

Thanks, @eddumelendez.

As far as I can tell, there's a general lifecycle problem with @Bean methods that inject DynamicPropertyRegistry and expect other beans to pick up any properties that are added to the registry. If the bean that adds properties to the registry hasn't been created at the point when a consumer of those properties is being created, the properties won't be visible. That's what's happening in your example.

The OtlpProperties bean is required during bean post-processing as it's a transitive dependency of tracingAwareMeterObservationHandler that's being called by ObservationRegistryPostProcessor when it's asked to post-process the observationRegistry bean. This means that the OtlpProperties bean is created and properties bound to it quite early in the context's lifecycle and, crucially, before the lgtmContainer @Bean method has been called. TestcontainersLifecycleBeanPostProcessor is called before ObservationRegistryPostProcessor to post-process observationRegistry but the bean factory's configuration has not be frozen by this point so it is not an opportunity to start all Testcontainer beans.

@ServiceConnection avoids the problem as it sets up some dependency relationships that are enough for the lgtmContainer bean to be created before the OtlpProperties bean is being created. Another workaround is to declare a dependency using a bean factory post-processor:

@Bean
static BeanFactoryPostProcessor otlpPropertiesDependencies() {
	return (beanFactory) -> {
		BeanDefinition definition = beanFactory.getBeanDefinition("management.otlp.metrics.export-" + OtlpProperties.class.getCanonicalName());
		definition.setDependsOn("lgtmContainer");
	};
}

I'm not sure how to fix this properly so I'd like to discuss it with the team. There are some similarities with problems we've had in the past with meter registry post-processing. The fixes for those may give us some inspiration.

@wilkinsona wilkinsona removed their assignment Jul 1, 2024
@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jul 1, 2024
@eddumelendez
Copy link
Contributor Author

Hi @wilkinsona, thanks for the explanation and the workaround! Looking forward for the team meeting's outcome

@philwebb philwebb added type: bug A general bug 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 Jul 3, 2024
@philwebb philwebb added this to the 3.2.x milestone Jul 3, 2024
@wilkinsona
Copy link
Member

We've fixed this in 3.4 with the switch to using DynamicPropertyRegistrar. Unfortunately, I don't think there's anything more was can do in a patch release so I'm closing this as a duplicate of #41996.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Oct 18, 2024
@wilkinsona wilkinsona removed this from the 3.2.x milestone Oct 18, 2024
@wilkinsona wilkinsona added the status: duplicate A duplicate of another issue label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants