Move autoconfigure getConfig to internal, remove getResource #5467

jack-berg · 2023-05-19T15:20:51Z

Related to #5464.

Moved AutoConfiguredOpenTelemetrySdk#getConfig() method to internal accessor to avoid backwards compatibility guarantees when we stabilize. Remove AutoConfiguredOpenTelemetrySdk#getResource() method.

Why remove getResource(): with Add public API to access environment resource #5554 merged, I don't think there are valid use cases that require accessing the autoconfigured resource to be accessed outside the configured providers.
Why move getConfig(): I'm worried about how this API / behavior of this method may need to change when we have file based configuration. If someone includes the environment variable that triggers file based configuration, there are still system properties and environment variables present, but which are ignored during configuration. Not sure what we should return to users in this scenario. Moving this to internal allows us to stabilize now without having to address a thorny question which we'll only want to address once we have a concrete implementation.

**Edited 6/22 **

codecov · 2023-05-19T15:27:01Z

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.02 ⚠️

Comparison is base (bcec7e9) 91.38% compared to head (9f94e29) 91.36%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5467      +/-   ##
============================================
- Coverage     91.38%   91.36%   -0.02%     
- Complexity     4964     4965       +1     
============================================
  Files           551      552       +1     
  Lines         14571    14576       +5     
  Branches       1358     1358              
============================================
+ Hits          13315    13318       +3     
- Misses          868      870       +2     
  Partials        388      388

Impacted Files	Coverage Δ
.../autoconfigure/AutoConfiguredOpenTelemetrySdk.java	`100.00% <ø> (ø)`
.../sdk/autoconfigure/internal/AutoConfigureUtil.java	`60.00% <60.00%> (ø)`

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

jkwatson · 2023-05-22T15:31:16Z

...nfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdk.java

@@ -51,10 +51,10 @@ static AutoConfiguredOpenTelemetrySdk create(
  public abstract OpenTelemetrySdk getOpenTelemetrySdk();

  /** Returns the {@link Resource} that was auto-configured. */
-  public abstract Resource getResource();
+  abstract Resource getResource();


Doesn't this remove the only supported way to get an autoconfigured Resource? In cases where people want to manually configure everything except the Resource, I believe this was the only possible way to do it. This is a useful thing to be able to do, and this change makes it even harder to access.

I've been a strong proponent of having a way to get access to an autoconfigured Resource instance, and have had to settle for this being the only way. This change would now make it impossible to do in a supported way.

If we're going to remove this, we should provide a supported way for users to get the resource.

Doesn't this remove the only supported way to get an autoconfigured Resource?

Well its been moved to an internal API, but yes, it removes it from the proposed stable API.

Can you remind me of some of the use cases of accessing the autoconfigured resource? My motivation for this is to be conservative about the public API surface area, and I struggled to think of reasons why you would want to access the resource.

My use-case is: I don't want to use the full auto-configure mechanisms because getting it right for my use-case is more trouble than just writing a few lines of java code to do it. But, I do want the auto-configured resource, since that's pretty simple.

@jkwatson this is related to #5238?

@trask Yes, indeed [at least to the conversation in that issue]

Now that #5554 is merged and there is a public API for obtaining the resource configured from environment variables / system properties, I've updated this PR to remove even the experimental accessor to get the autoconfigured resource.

With the new public API, I feel fairly strongly that any code accessing AutoConfiguredOpenTelemetrySdk.getResource() shouldn't.

Resource configured from env vars != the full autoconfigured resource

I'm quite certain that we will continue to use the AutoConfiguredOpenTelemetrySdk#getResource() method to get the full resource; we'll just have to move the reflection logic to our distro.

Anyway, I don't want to block this issue on one vendor's complaints (some of which might disappear in the future, when we have a profiling signal and SDK); even more when we have a workaround for that.

Resource configured from env vars != the full autoconfigured resource

Correct. I'm torn on this. I'm ok with re-adding an internal method to continue accessing the full autoconfigured resource, but feel that long term, folks should work exclusively through the providers. I worry that keeping an internal method gives hope that someday accessing the full autoconfigured resource will become a public API, where in actuality I think its more likely to go away altogether.

feel that long term, folks should work exclusively through the providers.

Yeah, I totally agree with that. Long-term (or maybe even mid-term, once we have the profiling SDK) our (Splunk's) use cases will go away; and with #5554 implemented SDK users themselves should have no reason for that getResource() method either.
I'm in favor of merging this PR as it is -- there are workaround that we can use in the Splunk distro; and the cause (stable autoconfigure module) is important enough.

jack-berg · 2023-06-20T14:50:58Z

Should resolve #5554 before merging this.

…y-java into internalize-autoconfigure-methods

mateuszrzeszutek · 2023-06-23T13:06:57Z

...nfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdk.java

@@ -51,10 +51,10 @@ static AutoConfiguredOpenTelemetrySdk create(
  public abstract OpenTelemetrySdk getOpenTelemetrySdk();

  /** Returns the {@link Resource} that was auto-configured. */
-  public abstract Resource getResource();
+  abstract Resource getResource();


Anyway, I don't want to block this issue on one vendor's complaints (some of which might disappear in the future, when we have a profiling signal and SDK); even more when we have a workaround for that.

jack-berg · 2023-06-27T16:05:27Z

@jkwatson WDYT?

jkwatson

Thanks!

Move autoconfigure getResource and getConfig to internal

6829478

jack-berg requested a review from a team May 19, 2023 15:20

jkwatson reviewed May 22, 2023

View reviewed changes

jack-berg mentioned this pull request May 30, 2023

Add EnvironmentResourceProvider open-telemetry/opentelemetry-java-instrumentation#8604

Closed

jack-berg mentioned this pull request Jun 16, 2023

Move environment resource to :sdk:all #5544

Closed

jack-berg added 2 commits June 22, 2023 16:31

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

d32012e

…y-java into internalize-autoconfigure-methods

Remove internal accessor for autoconfigured resource

9f94e29

jack-berg changed the title ~~Move autoconfigure getResource and getConfig to internal~~ Move autoconfigure getConfig to internal, remove getResource Jun 22, 2023

mateuszrzeszutek approved these changes Jun 23, 2023

View reviewed changes

jkwatson approved these changes Jun 27, 2023

View reviewed changes

jack-berg merged commit 73597b1 into open-telemetry:main Jun 27, 2023

jack-berg mentioned this pull request Jun 27, 2023

Mark opentelemetry-sdk-extension-autoconfigure as stable #5577

Merged

trask mentioned this pull request Jul 14, 2023

Bump io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha from 1.27.0-alpha to 1.28.0-alpha open-telemetry/opentelemetry-java-contrib#952

Merged

trask mentioned this pull request Mar 25, 2024

Make it easier for agent extensions to get the autoconfigured Resource #3924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move autoconfigure getConfig to internal, remove getResource #5467

Move autoconfigure getConfig to internal, remove getResource #5467

jack-berg commented May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

jkwatson May 22, 2023 •

edited

Loading

jack-berg May 22, 2023

jkwatson May 22, 2023

trask May 23, 2023

jkwatson May 23, 2023

jack-berg Jun 22, 2023

mateuszrzeszutek Jun 23, 2023

mateuszrzeszutek Jun 23, 2023

jack-berg Jun 23, 2023

mateuszrzeszutek Jun 26, 2023

jack-berg commented Jun 20, 2023

mateuszrzeszutek Jun 23, 2023

jack-berg commented Jun 27, 2023

jkwatson left a comment

Move autoconfigure getConfig to internal, remove getResource #5467

Move autoconfigure getConfig to internal, remove getResource #5467

Conversation

jack-berg commented May 19, 2023 • edited Loading

codecov bot commented May 19, 2023 • edited Loading

Codecov Report

jkwatson May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg commented Jun 20, 2023

Choose a reason for hiding this comment

jack-berg commented Jun 27, 2023

jkwatson left a comment

Choose a reason for hiding this comment

jack-berg commented May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

jkwatson May 22, 2023 •

edited

Loading