Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -100,6 +100,7 @@ public ResourceProvider otelDistroVersionResourceProvider() {
}

@Bean
@ConditionalOnMissingBean
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

i added this to allow people to customize the behavior, if they wanted

Copy link
Member

Choose a reason for hiding this comment

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

we already have

- which I feel is enough, but feel free to create a separate PR to discuss

Copy link
Member Author

Choose a reason for hiding this comment

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

in the original issue #15009 they asked to be able to override this:

The auto-configuration should allow overriding or replacing SpringConfigProperties gracefully, for example by supporting @ConditionalOnMissingBean, so users can provide a cached or customized implementation without having to fork the library.

But since we addressed the performance issue, perhaps it's less necessary?

Copy link
Member

Choose a reason for hiding this comment

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

yes, I think so

adding @ConditionalOnMissingBean increases the API surface area in ways that also increase complexity

public AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk(
Environment env,
OtlpExporterProperties otlpExporterProperties,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.spring.autoconfigure.internal.properties;

import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import javax.annotation.Nullable;
import org.springframework.core.env.Environment;

/**
* A caching wrapper around Spring's {@link Environment} that caches property lookups to avoid
* repeated expensive operations and property source traversal.
*
* <p>Thread-safe for concurrent access. Cached values persist indefinitely and are assumed to be
* immutable after the first access. If properties can change at runtime, use {@link #clear()} to
* invalidate the cache.
*
* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
* at any time.
*/
final class CachedPropertyResolver {
private final Environment environment;
private final ConcurrentHashMap<CacheKey, Optional<?>> cache = new ConcurrentHashMap<>();

CachedPropertyResolver(Environment environment) {
this.environment = Objects.requireNonNull(environment, "environment");
}

/**
* Gets a property value from the environment, using a cache to avoid repeated lookups.
*
* @param name the property name
* @param targetType the target type to convert to
* @return the property value, or null if not found
*/
@Nullable
<T> T getProperty(String name, Class<T> targetType) {
CacheKey key = new CacheKey(name, targetType);
// CacheKey includes targetType, ensuring type match
@SuppressWarnings("unchecked")
Optional<T> result =
(Optional<T>)
cache.computeIfAbsent(
key, k -> Optional.ofNullable(environment.getProperty(name, targetType)));
return result.orElse(null);
}

/**
* Gets a string property value from the environment.
*
* @param name the property name
* @return the property value, or null if not found
*/
@Nullable
String getProperty(String name) {
return getProperty(name, String.class);
}

/** Clears all cached property values, forcing fresh lookups on subsequent calls. */
void clear() {
cache.clear();
}

/** Cache key combining property name and target type. */
private static final class CacheKey {
private final String name;
private final Class<?> targetType;
private final int cachedHashCode;

CacheKey(String name, Class<?> targetType) {
this.name = Objects.requireNonNull(name, "name");
this.targetType = Objects.requireNonNull(targetType, "targetType");
this.cachedHashCode = Objects.hash(name, targetType);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (!(obj instanceof CacheKey)) {
return false;
}
CacheKey other = (CacheKey) obj;
return name.equals(other.name) && targetType.equals(other.targetType);
}

@Override
public int hashCode() {
return cachedHashCode;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
* any time.
*/
public class SpringConfigProperties implements ConfigProperties {
private final Environment environment;
private final CachedPropertyResolver cachedEnvironment;

private final ExpressionParser parser;
private final OtlpExporterProperties otlpExporterProperties;
Expand All @@ -44,7 +44,7 @@ public SpringConfigProperties(
OtelResourceProperties resourceProperties,
OtelSpringProperties otelSpringProperties,
ConfigProperties otelSdkProperties) {
this.environment = environment;
this.cachedEnvironment = new CachedPropertyResolver(environment);
this.parser = parser;
this.otlpExporterProperties = otlpExporterProperties;
this.resourceProperties = resourceProperties;
Expand Down Expand Up @@ -152,10 +152,10 @@ public static ConfigProperties create(
@Override
public String getString(String name) {
String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);
String value = environment.getProperty(normalizedName, String.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative approach that could be considered would be to wrap the environment into a caching PropertyResolver. I presume that the sdk deliberately rereads these properties so they could be updated with opamp in the future. Idk whether this is something we should be concerned with.

Copy link
Member

Choose a reason for hiding this comment

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

I presume that the sdk deliberately rereads these properties

no, it was not a deliberate decision as far as I know.

When I worked on the project, I assumed that properties would only be read at startup time.

I think it's hard to anticipate how OpAMP will be integrated, but we may simply flush the cache (from the caching PropertyResolver.

Copy link
Member Author

Choose a reason for hiding this comment

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

implemented a caching property resolver, and included a way to flush the cache

String value = cachedEnvironment.getProperty(normalizedName, String.class);
if (value == null && normalizedName.equals("otel.exporter.otlp.protocol")) {
// SDK autoconfigure module defaults to `grpc`, but this module aligns with recommendation
// in specification to default to `http/protobuf`
// SDK autoconfigure module defaults to `grpc`, but this module aligns with
// recommendation in specification to default to `http/protobuf`
return OtlpConfigUtil.PROTOCOL_HTTP_PROTOBUF;
}
return or(value, otelSdkProperties.getString(name));
Expand All @@ -165,38 +165,40 @@ public String getString(String name) {
@Override
public Boolean getBoolean(String name) {
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Boolean.class),
cachedEnvironment.getProperty(
ConfigUtil.normalizeEnvironmentVariableKey(name), Boolean.class),
otelSdkProperties.getBoolean(name));
}

@Nullable
@Override
public Integer getInt(String name) {
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Integer.class),
cachedEnvironment.getProperty(
ConfigUtil.normalizeEnvironmentVariableKey(name), Integer.class),
otelSdkProperties.getInt(name));
}

@Nullable
@Override
public Long getLong(String name) {
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Long.class),
cachedEnvironment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Long.class),
otelSdkProperties.getLong(name));
}

@Nullable
@Override
public Double getDouble(String name) {
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Double.class),
cachedEnvironment.getProperty(
ConfigUtil.normalizeEnvironmentVariableKey(name), Double.class),
otelSdkProperties.getDouble(name));
}

@SuppressWarnings("unchecked")
@Override
public List<String> getList(String name) {

String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);

List<String> list = listPropertyValues.get(normalizedName);
Expand All @@ -210,7 +212,9 @@ public List<String> getList(String name) {
}
}

return or(environment.getProperty(normalizedName, List.class), otelSdkProperties.getList(name));
List<String> envValue =
(List<String>) cachedEnvironment.getProperty(normalizedName, List.class);
return or(envValue, otelSdkProperties.getList(name));
}

@Nullable
Expand All @@ -231,6 +235,21 @@ public Map<String, String> getMap(String name) {

String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);
// maps from config properties are not supported by Environment, so we have to fake it
Map<String, String> specialMap = getSpecialMapProperty(normalizedName, otelSdkMap);
if (specialMap != null) {
return specialMap;
}

String value = cachedEnvironment.getProperty(normalizedName);
if (value == null) {
return otelSdkMap;
}
return (Map<String, String>) parser.parseExpression(value).getValue();
}

@Nullable
private Map<String, String> getSpecialMapProperty(
String normalizedName, Map<String, String> otelSdkMap) {
switch (normalizedName) {
case "otel.resource.attributes":
return mergeWithOtel(resourceProperties.getAttributes(), otelSdkMap);
Expand All @@ -243,14 +262,8 @@ public Map<String, String> getMap(String name) {
case "otel.exporter.otlp.traces.headers":
return mergeWithOtel(otlpExporterProperties.getTraces().getHeaders(), otelSdkMap);
default:
break;
return null;
}

String value = environment.getProperty(normalizedName);
if (value == null) {
return otelSdkMap;
}
return (Map<String, String>) parser.parseExpression(value).getValue();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.exporter.otlp.internal.OtlpSpanExporterProvider;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import io.opentelemetry.sdk.autoconfigure.spi.internal.AutoConfigureListener;
import io.opentelemetry.sdk.trace.export.SpanExporter;
Expand Down Expand Up @@ -58,6 +59,27 @@ void customOpenTelemetry() {
.hasBean("otelProperties"));
}

private static class CustomAutoConfiguredSdkConfiguration {
@Bean
public AutoConfiguredOpenTelemetrySdk customAutoConfiguredSdk() {
return AutoConfiguredOpenTelemetrySdk.builder().build();
}
}

@Test
@DisplayName(
"when Application Context contains AutoConfiguredOpenTelemetrySdk bean should NOT use default")
void customAutoConfiguredOpenTelemetrySdk() {
this.contextRunner
.withUserConfiguration(CustomAutoConfiguredSdkConfiguration.class)
.withConfiguration(AutoConfigurations.of(OpenTelemetryAutoConfiguration.class))
.run(
context ->
assertThat(context)
.hasBean("customAutoConfiguredSdk")
.doesNotHaveBean("autoConfiguredOpenTelemetrySdk"));
}

@Test
@DisplayName(
"when Application Context DOES NOT contain OpenTelemetry bean should initialize openTelemetry")
Expand Down
Loading
Loading