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

Spring Boot Starter service-name is constant #5359

Conversation

aschugunov
Copy link
Contributor

Pattern-based resource attributes configuration
fix 3480

@aschugunov aschugunov requested a review from a team February 11, 2022 23:18
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 11, 2022

CLA Signed

The committers are authorized under a signed CLA.

@aschugunov
Copy link
Contributor Author

@mateuszrzeszutek could you provide some feedback, please?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx @aschugunov! sorry for the delay in reviewing

instrumentation/spring/spring-boot-autoconfigure/README.md Outdated Show resolved Hide resolved
instrumentation/spring/spring-boot-autoconfigure/README.md Outdated Show resolved Hide resolved
instrumentation/spring/spring-boot-autoconfigure/README.md Outdated Show resolved Hide resolved
Comment on lines 52 to 47
public Supplier<Resource> otelOsResourceProvider() {
return OsResource::get;
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I dont deeply understand how SPI works.
Currently, trace does not contain any inforamtion about os, container and other resources.
Main idea is combine all resources via supplier resource beans and then inject complex resource into tracerProvider.
maybe it is possbile to do with SPI, I will try to research. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to load resource objects via spring.factories and it seems spring does not load bean by interface or I did it in wrong manner.
If I understand correctly java loads objects via SPI when implementation is located in classpath, spring can customize context via spring.factories file. But this way does not work

io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider=\
io.opentelemetry.sdk.extension.resources.ContainerResourceProvider,\
io.opentelemetry.sdk.extension.resources.HostResourceProvider,\
io.opentelemetry.sdk.extension.resources.OsResourceProvider,\
io.opentelemetry.sdk.extension.resources.ProcessResourceProvider,\
io.opentelemetry.sdk.extension.resources.ProcessRuntimeResourceProvider

I added @ConditionalOnClass over appropriate bean, maybe it will be more suitable

Copy link
Member

Choose a reason for hiding this comment

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

How about registering the ResourceProviders as beans? That's the same interface that OTel SDK autoconfigure uses - but instead of using SPI/ServiceLoader you'd just load all beans of that type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain a bit more, please?
Did you mean to create resourceProvider beans in addition to resource beans?

Copy link
Member

Choose a reason for hiding this comment

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

So, instead of all the Supplier<Resource> beans in this class, you could use ResourceProvider instead:

  @Bean
  @ConditionalOnClass(OsResource.class)
  public ResourceProvider osResourceProvider() {
    return new OsResourceProvider();
  }

  @Bean
  @ConditionalOnClass(ProcessResource.class)
  public ResourceProvider processResourceProvider() {
    return new ProcessResourceProvider();
  }

 // ...

Then, in the OpenTelemetryAutoConfiguration class you'd change the otelResource method so that it uses these beans:

    @Bean
    @ConditionalOnMissingBean
    public Resource otelResource(
        Environment env, ObjectProvider<List<ResourceProvider>> resourceProviders) {
      String applicationName = env.getProperty("spring.application.name");
      Resource resource = defaultResource(applicationName);
      ConfigProperties config = SpringConfigProperties(env);
      for (ResourceProvider provider : resourceProviders.getIfAvailable(Collections::emptyList)) {
        resource = resource.merge(provider.createResource(config));
      }
      return resource;
    }

ResourceProvider#createResource requires a ConfigProperties argument, but it would be quite simple to implement one that just simply delegates to Spring's Environment class.

This way you could reuse the SDK ResourceProvider interface in a way that's compatible with Spring beans -- instead of using SPI, the user of the instrumented application could just configure a ResourceProvider bean in their app and it'd get picked up automatically by this lib.

WDYT about that?

Copy link
Contributor Author

@aschugunov aschugunov Mar 28, 2022

Choose a reason for hiding this comment

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

Yes it is a great idea to reuse existed interface for getting all available resources.
I see benefit of interface, it can be more clear for whole open telemetry project because interface provides consistent concept.
On the other hand, supplier follows the same idea - to provide resource. I mean there is no difference how spring will provide beans - via ResourceProvider interface or via Supplier<Resource>. Also interface enforce to use ConfigProperties that is not used in any current resourceProviders implementations.
OtelResourceProperties is such type of config. It is created by spring automatically.

If I understand correctly I should implement something like SpringResourceProvider and encapsulate there logic of determination of application name and attributes.

I will add these changes in second commit in this PR in a day or two. So you will be able to evaluate changes and choose more appropriate implementation.

Copy link
Member

Choose a reason for hiding this comment

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

I will add these changes in second commit in this PR in a day or two. So you will be able to evaluate changes and choose more appropriate implementation.

👍 Thanks!

@aschugunov aschugunov force-pushed the issue-3480-service-name-is-constant branch 3 times, most recently from a5595cd to db474af Compare March 13, 2022 07:52
@aschugunov aschugunov force-pushed the issue-3480-service-name-is-constant branch from db474af to a6868af Compare March 14, 2022 06:25
@aschugunov aschugunov force-pushed the issue-3480-service-name-is-constant branch 2 times, most recently from d7977d8 to f0b71d1 Compare March 15, 2022 05:57
@trask
Copy link
Member

trask commented Mar 17, 2022

@open-telemetry/java-instrumentation-approvers last chance to review!

Comment on lines 77 to 83
private static Resource defaultResource(String applicationName) {
if (applicationName == null) {
return Resource.getDefault();
}
return Resource.getDefault()
.merge(Resource.create(Attributes.of(ResourceAttributes.SERVICE_NAME, applicationName)));
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

Comment on lines 52 to 47
public Supplier<Resource> otelOsResourceProvider() {
return OsResource::get;
}
Copy link
Member

Choose a reason for hiding this comment

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

How about registering the ResourceProviders as beans? That's the same interface that OTel SDK autoconfigure uses - but instead of using SPI/ServiceLoader you'd just load all beans of that type.

Pattern-based resource configuration
@aschugunov aschugunov force-pushed the issue-3480-service-name-is-constant branch from f0b71d1 to 53678f2 Compare March 25, 2022 05:05
@aschugunov aschugunov force-pushed the issue-3480-service-name-is-constant branch from d7c8011 to 393e0b9 Compare March 29, 2022 10:10
@mateuszrzeszutek mateuszrzeszutek merged commit c5f677e into open-telemetry:main Mar 29, 2022
@mateuszrzeszutek
Copy link
Member

Thanks @aschugunov !

RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Spring Boot Starter service-name is constant

Pattern-based resource configuration

* Add ResourceProvider beans for spring with ConfigProperties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spring Boot Starter service-name is constant
3 participants