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

Merge the current Resource object with custom Resource Object. #5619

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

parth1601
Copy link
Contributor

@parth1601 parth1601 commented Jul 11, 2023

Merge the current Resource object with custom Resource Object.

Issue

  • Needs to add some custom resource attribute to the specific SdkProviderBuilder (Logger, Meter, Tracer).

Solution

  • Added one method mergeResource to the all SdkProviderBuilder that takes an object od Resource Class and merge with the current one.
  • Before
    @AutoService(AutoConfigurationCustomizerProvider.class)
    public class DemoAutoConfigurationCustomizerProvider
        implements AutoConfigurationCustomizerProvider {
      @Override
      public void customize(AutoConfigurationCustomizer autoConfiguration) {
        autoConfiguration
            .addMeterProviderCustomizer(this::configureSdkMeterProvider)
            ...
      }
    
      private SdkMeterProviderBuilder configureSdkMeterProvider(
          SdkMeterProviderBuilder meterProvider, ConfigProperties config) {
        try {
          Field resourceField = meterProvider.getClass().getDeclaredField("resource");
          resourceField.setAccessible(true);
          Resource resource = (Resource) resourceField.get(meterProvider);
          meterProvider.setResource(
              resource.merge(
                  Resource.create(
                      Attributes.of(
                          AttributeKey.stringKey("custom.attribute.key.1"), "custom.attribute.value.1",
                          AttributeKey.stringKey("custom.attribute.key.2"), "custom.attribute.value.2"
                      )
                  )
              )
          );
        } catch (Exception e) {
          throw new RuntimeException(e);
        }
        return meterProvider;
      }
      
      ...
    }
  • After
    @AutoService(AutoConfigurationCustomizerProvider.class)
    public class DemoAutoConfigurationCustomizerProvider
        implements AutoConfigurationCustomizerProvider {
      @Override
      public void customize(AutoConfigurationCustomizer autoConfiguration) {
        autoConfiguration
            .addMeterProviderCustomizer(this::configureSdkMeterProvider)
            ...
      }
    
      private SdkMeterProviderBuilder configureSdkMeterProvider(
          SdkMeterProviderBuilder meterProvider, ConfigProperties config) {
        meterProvider.addResource(
            Resource.create(
                Attributes.of(
                    AttributeKey.stringKey("custom.attribute.key.1"), "custom.attribute.value.1",
                    AttributeKey.stringKey("custom.attribute.key.2"), "custom.attribute.value.2"
                )
            )
        );
        return meterProvider;
      }
      
      ...
    }

@parth1601 parth1601 requested a review from a team July 11, 2023 05:58
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 11, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.08 ⚠️

Comparison is base (9ecc6f0) 91.31% compared to head (c9171df) 91.23%.

❗ Current head c9171df differs from pull request most recent head 288ee22. Consider uploading reports for the commit 288ee22 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5619      +/-   ##
============================================
- Coverage     91.31%   91.23%   -0.08%     
+ Complexity     4982     4980       -2     
============================================
  Files           554      554              
  Lines         14749    14758       +9     
  Branches       1376     1376              
============================================
- Hits          13468    13465       -3     
- Misses          887      897      +10     
- Partials        394      396       +2     
Impacted Files Coverage Δ
...entelemetry/sdk/logs/SdkLoggerProviderBuilder.java 85.71% <0.00%> (-14.29%) ⬇️
...telemetry/sdk/metrics/SdkMeterProviderBuilder.java 90.32% <0.00%> (-9.68%) ⬇️
...ntelemetry/sdk/trace/SdkTracerProviderBuilder.java 81.25% <0.00%> (-8.41%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zeitlinger
Copy link
Member

zeitlinger commented Jul 11, 2023

@open-telemetry/java-approvers

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md does not mention this use case - should it be added?

@jack-berg
Copy link
Member

The use case you describe in the PR description is already possible in a couple of different ways.

You can add a resource customizer:

AutoConfiguredOpenTelemetrySdk.builder()
  .addResourceCustomizer((resource, config) -> resource.merge(Resource.create(...)));

Or you can define a custom resource provider:

  @AutoService(ResourceProvider.class)
  public static class MyResourceProvider implements ResourceProvider {
    @Override
    public Resource createResource(ConfigProperties config) {
      return Resource.create(...);
    }
  }

I don't think we need additional syntactic sugar to provide another way to achieve the same thing.

@parth1601
Copy link
Contributor Author

Hi @jack-berg ,
Thank you for this suggetion.

IMO, this is the issue what you explain, this will update the resource at globle level (For all SdkProviderBuilder like Log, Metrics and Traces), but I want to customize the resource of any one of them (As an example SdkMeterProviderBuilder) then I think as of now there is no such a way, if it already exist then please provide your suggetion.

@breedx-splk
Copy link
Contributor

IMO, this is the issue what you explain, this will update the resource at globle level (For all SdkProviderBuilder like Log, Metrics and Traces), but I want to customize the resource of any one of them (As an example SdkMeterProviderBuilder) then I think as of now there is no such a way, if it already exist then please provide your suggetion.

I don't have a citation from the spec handy, but I'm pretty sure you should NOT be using different Resources for different signal types within the same sdk instance. That's a recipe for disaster for any backend that is trying to do correlation or otherwise link between signals.

@parth1601
Copy link
Contributor Author

Hello @breedx-splk,
Actually, I have one use case, where I have to add some custom attributes (for filtering purpose) only for the metrics not for Traces and Logs. That's why I have to do that. Hope you will get it.

Note : I don't want to change the whole resource I just want to add some attributes over there.

There is another way to archive this,

public SdkTracerProviderBuilder addAttributes(Attributes attributes) {
    if (!attributes.isEmpty()) {
      this.resource = this.resource.toBuilder().putAll(attributes).build();
    }
    return this;
}

But at the end have to add method to append the attributes from any of the way.

@jack-berg
Copy link
Member

@parth1601 see my comment in a related issue.

In summary, I support the addition of this method, but generally don't recommend having a resource that differs by signal. The addition of this method would help alleviate accidental removal of the default resource attributes, which I'm supportive of.

@jkwatson
Copy link
Contributor

Should the new method(s) be called "mergeResource" or "addResource" do you think?

@trask
Copy link
Member

trask commented Jul 13, 2023

Smallish +1 for addResource over mergeResource

@parth1601
Copy link
Contributor Author

Changed the method name to addResource.

@jack-berg
Copy link
Member

@jkwatson take a look when you can. Would be good to get this in for the 1.29.0 release.

@jack-berg jack-berg merged commit f52050b into open-telemetry:main Aug 9, 2023
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.

6 participants