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

Move autoconfigure getConfig to internal, remove getResource #5467

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

@jkwatson jkwatson May 22, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@jkwatson this is related to #5238?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


/** Returns the {@link ConfigProperties} used for auto-configuration. */
public abstract ConfigProperties getConfig();
abstract ConfigProperties getConfig();

AutoConfiguredOpenTelemetrySdk() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.autoconfigure.internal;

import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;

/**
* This class is internal and is hence not for public use. Its APIs are unstable and can change at
* any time.
*/
public final class AutoConfigureUtil {

private AutoConfigureUtil() {}

/** Returns the {@link ConfigProperties} used for auto-configuration. */
public static ConfigProperties getConfig(
AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) {
try {
Method method = AutoConfiguredOpenTelemetrySdk.class.getDeclaredMethod("getConfig");
method.setAccessible(true);
return (ConfigProperties) method.invoke(autoConfiguredOpenTelemetrySdk);
} catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
throw new IllegalStateException(

Check warning on line 29 in sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/internal/AutoConfigureUtil.java

View check run for this annotation

Codecov / codecov/patch

sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/internal/AutoConfigureUtil.java#L28-L29

Added lines #L28 - L29 were not covered by tests
"Error calling getConfig on AutoConfiguredOpenTelemetrySdk", e);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.autoconfigure;

import static java.util.Collections.singletonMap;

import io.opentelemetry.sdk.autoconfigure.internal.AutoConfigureUtil;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

class AutoConfigureUtilTest {

AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk;

@BeforeEach
void beforeEach() {
autoConfiguredOpenTelemetrySdk =
AutoConfiguredOpenTelemetrySdk.builder()
.addPropertiesSupplier(() -> singletonMap("otel.metrics.exporter", "none"))
.addPropertiesSupplier(() -> singletonMap("otel.traces.exporter", "none"))
.addPropertiesSupplier(() -> singletonMap("otel.logs.exporter", "none"))
.build();
}

@Test
void getConfig() {
Assertions.assertThat(AutoConfigureUtil.getConfig(autoConfiguredOpenTelemetrySdk))
.isSameAs(autoConfiguredOpenTelemetrySdk.getConfig());
}
}