-
Notifications
You must be signed in to change notification settings - Fork 1k
[Spring starter] Add cache to reduce environment variable lookups #15132
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
Changes from 5 commits
9d975b9
39757e1
5fa3e39
77e4485
4dd742c
00b5261
d00e326
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ | |
| * any time. | ||
| */ | ||
| public class SpringConfigProperties implements ConfigProperties { | ||
| private final Environment environment; | ||
| private final CachedPropertyResolver environment; | ||
|
|
||
| private final ExpressionParser parser; | ||
| private final OtlpExporterProperties otlpExporterProperties; | ||
|
|
@@ -44,7 +44,7 @@ public SpringConfigProperties( | |
| OtelResourceProperties resourceProperties, | ||
| OtelSpringProperties otelSpringProperties, | ||
| ConfigProperties otelSdkProperties) { | ||
| this.environment = environment; | ||
| this.environment = new CachedPropertyResolver(environment); | ||
| this.parser = parser; | ||
| this.otlpExporterProperties = otlpExporterProperties; | ||
| this.resourceProperties = resourceProperties; | ||
|
|
@@ -154,8 +154,8 @@ public String getString(String name) { | |
| String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name); | ||
| String value = environment.getProperty(normalizedName, String.class); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative approach that could be considered would be to wrap the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| 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)); | ||
|
|
@@ -196,7 +196,6 @@ public Double getDouble(String name) { | |
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public List<String> getList(String name) { | ||
|
|
||
| String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name); | ||
|
|
||
| List<String> list = listPropertyValues.get(normalizedName); | ||
|
|
@@ -210,7 +209,8 @@ public List<String> getList(String name) { | |
| } | ||
| } | ||
|
|
||
| return or(environment.getProperty(normalizedName, List.class), otelSdkProperties.getList(name)); | ||
| List<String> envValue = (List<String>) environment.getProperty(normalizedName, List.class); | ||
| return or(envValue, otelSdkProperties.getList(name)); | ||
| } | ||
|
|
||
| @Nullable | ||
|
|
@@ -231,6 +231,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 = environment.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); | ||
|
|
@@ -243,14 +258,8 @@ public Map<String, String> getMap(String name) { | |
| case "otel.exporter.otlp.traces.headers": | ||
| return mergeWithOtel(otlpExporterProperties.getTraces().getHeaders(), otelSdkMap); | ||
| default: | ||
| break; | ||
| } | ||
|
|
||
| String value = environment.getProperty(normalizedName); | ||
| if (value == null) { | ||
| return otelSdkMap; | ||
| return null; | ||
| } | ||
| return (Map<String, String>) parser.parseExpression(value).getValue(); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.