Skip to content
Open
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 @@ -5,13 +5,28 @@

package io.opentelemetry.contrib.dynamic.policy.registry;

import io.opentelemetry.contrib.dynamic.policy.registry.json.JsonPolicyInitConfigReader;
import io.opentelemetry.contrib.dynamic.policy.registry.yaml.YamlPolicyInitConfigReader;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nullable;

/** Top-level registry initialization model containing per-source mapping config. */
public final class PolicyInitConfig {
static final String POLICY_INIT_CONFIG_PROPERTY_JSON =
"otel.java.experimental.telemetry.policy.init.json";
static final String POLICY_INIT_CONFIG_PROPERTY_YAML =
"otel.java.experimental.telemetry.policy.init.yaml";
private static final Logger logger = Logger.getLogger(PolicyInitConfig.class.getName());

private final List<PolicySourceConfig> sources;

Expand All @@ -28,6 +43,52 @@ public List<PolicySourceConfig> getSources() {
return sources;
}

/**
* Reads policy-init configuration based on config properties.
*
* <p>YAML takes precedence over JSON when both are present. If both are present, and the YAML
* file is invalid, the JSON file is still ignored. If the file parsed is invalid, a warning is
* logged and null is returned.
*
* @param config OpenTelemetry config properties
* @return parsed init config, or null when no init-config path is configured or the file is
* invalid
* @throws NullPointerException if config is null
*/
@Nullable
public static PolicyInitConfig readFromConfigProperties(ConfigProperties config) {
Objects.requireNonNull(config, "config cannot be null");
String mappingPathYaml = config.getString(POLICY_INIT_CONFIG_PROPERTY_YAML);
if (mappingPathYaml == null || mappingPathYaml.trim().isEmpty()) {
String mappingPathJson = config.getString(POLICY_INIT_CONFIG_PROPERTY_JSON);
if (mappingPathJson == null || mappingPathJson.trim().isEmpty()) {
Comment on lines +59 to +64
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we use declarative config api instead? (and rely on the java agent declarative config bridge for the system property support)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that gives any benefit. The system property doesn't provide the pipeline configuration, it points to a file that provides pipeline configuration. We'd need to change the property to be an ugly string encoded yaml to use that. json would work because that can be a single string for the full config, but yaml would be ugly.

jsonconfig='{"sources": [{"kind": "opamp", "format": "jsonkeyvalue", "location": "elastic", "mappings": [ { "sourceKey": "sampling_rate", "policyType": "trace_sampling_rate_policy" }, ] } ]}'
yamlconfig=sources:\n  - kind: opamp\n    format: jsonkeyvalue\n    location: elastic\n    mappings:\n      - sourceKey: sampling_rate\n        policyType: trace_sampling_rate_policy

Even to define a small pipeline this is painful

The yaml is deliberately setup so that when the telemetry policy otep is accepted and we then add policy to declarative config, it slots right in, eg

policy:
 sources:
   - kind: opamp
     format: jsonkeyvalue
     location: elastic
     mappings:
       - sourceKey: sampling_rate
         policyType: trace_sampling_rate_policy

The node under policy is exactly what this is supporting. To use the declarative config api just to get the file path seems pointless?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The yaml is deliberately setup so that when the telemetry policy otep is accepted and we then add policy to declarative config, it slots right in, eg

policy:
 sources:
   - kind: opamp
     format: jsonkeyvalue
     location: elastic
     mappings:
       - sourceKey: sampling_rate
         policyType: trace_sampling_rate_policy

does it make sense to slot it in now, even if the location under declarative config changes later when it gets an official location?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can do that for the declarative config, since this is experimental it would be fine. But how would you define the config when declarative config is not used.

List<DeclarativeConfigProperties> sources = config.getStructuredList("sources");
if (sources == null) {//true when not using declarative config
  //how to get the structure? read a file? use the ugly stringified file?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could this be a declarative config only feature? (similar to rule based sampling, some of the new method instrumentation, ..)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Allowing the structure to be read by failing over to how this PR does it would work. So it would look like this. I need to add the additional methods if this plan looks okay

PolicyInitConfig policyPipeline;
List<DeclarativeConfigProperties> sources = config.getStructuredList("sources");
if (sources == null) {//true when not using declarative config
  policyPipeline = PolicyInitConfig.readFromConfigProperties();
} else {
  policyPipeline = convertToPipeline(sources);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've done a different implementation in #2766 (I'll close this PR when this discussion is complete or shifts to that).

I think having only declarative config supported imposes a very high burden here. You already need to include this as an extension, insisting that you must also wire everything up using declarative config in order to use this makes it a huge shift instead of the smaller one I'd like to support

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think having only declarative config supported imposes a very high burden here.

yeah, I understand. at the same time, the whole reason we (in Java at least) have been looking forward to declarative configuration is that it allows us to have a unified story for these kinds of complex configurations, instead of having lots of different config files that are pointed to by various configuration properties (e.g. having separate metric view yaml configuration file, rule-based sampling configuration file, jmx configuration file, opamp policy configuration file, methods instrumentation configuration file, etc etc..)

Copy link
Copy Markdown
Contributor Author

@jackshirazi jackshirazi Apr 17, 2026

Choose a reason for hiding this comment

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

I've made the declarative config the default in the new PR, with support for experimental config files only if declarative config is not used.

Without the config file failover, it just forces deployments that don't use declarative config to use another coded extension to create the pipeline which seems like imposing a lot of additional pain to avoid including an option and a method

return null;
} else {
try (InputStream in = Files.newInputStream(Paths.get(mappingPathJson.trim()))) {
return JsonPolicyInitConfigReader.read(in);
} catch (IOException | RuntimeException e) {
logger.log(
Level.WARNING,
"Failed to load telemetry policy init config from {0}",
mappingPathJson.trim());
logger.log(Level.WARNING, "Policy init config read failed", e);
return null;
}
}
} else {
try (InputStream in = Files.newInputStream(Paths.get(mappingPathYaml.trim()))) {
return YamlPolicyInitConfigReader.read(in);
} catch (IOException | RuntimeException e) {
logger.log(
Level.WARNING,
"Failed to load telemetry policy init config from {0}",
mappingPathYaml.trim());
logger.log(Level.WARNING, "Policy init config read failed", e);
return null;
}
}
}

Comment thread
jackshirazi marked this conversation as resolved.
@Override
public boolean equals(Object obj) {
if (this == obj) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.dynamic.policy.registry;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import io.opentelemetry.contrib.dynamic.policy.source.SourceFormat;
import io.opentelemetry.contrib.dynamic.policy.source.SourceKind;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

class PolicyInitConfigTest {

@TempDir Path tempDir;

@Test
void readFromConfigPropertiesReturnsNullWhenBothPathsMissing() {
ConfigProperties config = mock(ConfigProperties.class);
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)).thenReturn(null);
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON)).thenReturn(null);

assertThat(PolicyInitConfig.readFromConfigProperties(config)).isNull();
}

@Test
void readFromConfigPropertiesPrefersYamlWhenBothPathsPresent() throws Exception {
Path jsonPath = tempDir.resolve("policy-init.json");
Files.write(jsonPath, jsonWithLocation("from-json").getBytes(StandardCharsets.UTF_8));
Path yamlPath = tempDir.resolve("policy-init.yaml");
Files.write(yamlPath, yamlWithLocation("from-yaml").getBytes(StandardCharsets.UTF_8));

ConfigProperties config = mock(ConfigProperties.class);
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML))
.thenReturn(yamlPath.toString());
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON))
.thenReturn(jsonPath.toString());

PolicyInitConfig initConfig = PolicyInitConfig.readFromConfigProperties(config);

assertThat(initConfig).isNotNull();
assertThat(initConfig.getSources()).hasSize(1);
assertThat(initConfig.getSources().get(0).getLocation()).isEqualTo("from-yaml");
}

@Test
void readFromConfigPropertiesFallsBackToJsonWhenYamlBlank() throws Exception {
Path jsonPath = tempDir.resolve("policy-init.json");
Files.write(jsonPath, jsonWithLocation("from-json").getBytes(StandardCharsets.UTF_8));

ConfigProperties config = mock(ConfigProperties.class);
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)).thenReturn(" ");
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON))
.thenReturn(jsonPath.toString());

PolicyInitConfig initConfig = PolicyInitConfig.readFromConfigProperties(config);

assertThat(initConfig).isNotNull();
assertThat(initConfig.getSources()).hasSize(1);
assertThat(initConfig.getSources().get(0).getLocation()).isEqualTo("from-json");
}

@Test
void readFromConfigPropertiesReadsJsonWhenYamlPathMissing() throws Exception {
Path configPath = tempDir.resolve("policy-init.json");
Files.write(configPath, minimalJsonConfig().getBytes(StandardCharsets.UTF_8));

PolicyInitConfig config = configFromPaths(null, configPath.toString());

assertThat(config.getSources()).hasSize(1);
PolicySourceConfig source = config.getSources().get(0);
assertThat(source.getKind()).isEqualTo(SourceKind.OPAMP);
assertThat(source.getFormat()).isEqualTo(SourceFormat.JSONKEYVALUE);
assertThat(source.getLocation()).isEqualTo("vendor");
assertThat(source.getMappings()).hasSize(1);
assertThat(source.getMappings().get(0).getSourceKey()).isEqualTo("sampling_rate");
assertThat(source.getMappings().get(0).getPolicyType()).isEqualTo("trace_sampling_rate_policy");
}

@Test
void readFromConfigPropertiesReadsYamlWhenYamlPathProvided() throws Exception {
Path configPath = tempDir.resolve("policy-init.yaml");
Files.write(configPath, minimalYamlConfig().getBytes(StandardCharsets.UTF_8));

PolicyInitConfig config = configFromPaths(configPath.toString(), null);

assertThat(config.getSources()).hasSize(1);
PolicySourceConfig source = config.getSources().get(0);
assertThat(source.getKind()).isEqualTo(SourceKind.OPAMP);
assertThat(source.getFormat()).isEqualTo(SourceFormat.JSONKEYVALUE);
assertThat(source.getMappings()).hasSize(1);
}

@Test
void readFromConfigPropertiesReturnsNullOnFileReadFailure() {
Path missing = tempDir.resolve("missing-policy-init.json");

assertThat(configFromPaths(null, missing.toString())).isNull();
}

@Test
void readFromConfigPropertiesReturnsNullOnParseFailure() throws Exception {
Path badJson = tempDir.resolve("bad-policy-init.json");
Files.write(badJson, "{}".getBytes(StandardCharsets.UTF_8));

assertThat(configFromPaths(null, badJson.toString())).isNull();
}

private static PolicyInitConfig configFromPaths(String yamlPath, String jsonPath) {
ConfigProperties config = mock(ConfigProperties.class);
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)).thenReturn(yamlPath);
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON)).thenReturn(jsonPath);
return PolicyInitConfig.readFromConfigProperties(config);
}

private static String minimalJsonConfig() {
return "{\"sources\":[{\"kind\":\"opamp\",\"format\":\"jsonkeyvalue\",\"location\":\"vendor\","
+ "\"mappings\":[{\"sourceKey\":\"sampling_rate\",\"policyType\":\"trace_sampling_rate_policy\"}]}]}";
}

private static String jsonWithLocation(String location) {
return "{\"sources\":[{\"kind\":\"opamp\",\"format\":\"jsonkeyvalue\",\"location\":\""
+ location
+ "\",\"mappings\":[{\"sourceKey\":\"sampling_rate\",\"policyType\":\"trace_sampling_rate_policy\"}]}]}";
}

private static String minimalYamlConfig() {
return "sources:\n"
+ " - kind: opamp\n"
+ " format: jsonkeyvalue\n"
+ " location: vendor\n"
+ " mappings:\n"
+ " - sourceKey: sampling_rate\n"
+ " policyType: trace_sampling_rate_policy\n";
}

private static String yamlWithLocation(String location) {
return "sources:\n"
+ " - kind: opamp\n"
+ " format: jsonkeyvalue\n"
+ " location: "
+ location
+ "\n"
+ " mappings:\n"
+ " - sourceKey: sampling_rate\n"
+ " policyType: trace_sampling_rate_policy\n";
}
}
Loading