Skip to content

Commit ff23eb2

Browse files
jackshirazijackshirazi
committed
copilot feedback again
1 parent 6af1d6b commit ff23eb2

3 files changed

Lines changed: 94 additions & 15 deletions

File tree

‎dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/registry/PolicyInitConfig.java‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,10 @@ public List<PolicySourceConfig> getSources() {
7676
* }</pre>
7777
*
7878
* @param declarativeConfig declarative config root
79-
* @return parsed init config, or null when telemetry_policy/sources is not configured
79+
* @return parsed init config, or null when telemetry_policy is not configured
8080
* @throws NullPointerException if declarativeConfig is null
81-
* @throws IllegalArgumentException if telemetry_policy is present but invalid
81+
* @throws IllegalArgumentException if telemetry_policy is present but invalid (for example,
82+
* missing or empty sources)
8283
*/
8384
@Nullable
8485
public static PolicyInitConfig readFromDeclarativeConfigProperties(
@@ -92,7 +93,7 @@ public static PolicyInitConfig readFromDeclarativeConfigProperties(
9293
List<DeclarativeConfigProperties> sourceConfigs =
9394
telemetryPolicyConfig.getStructuredList(SOURCES_DECLARATIVE_KEY);
9495
if (sourceConfigs == null || sourceConfigs.isEmpty()) {
95-
return null;
96+
throw new IllegalArgumentException("Config must contain a non-empty 'sources' array.");
9697
}
9798

9899
List<PolicySourceConfig> sources = new ArrayList<>();
@@ -168,23 +169,15 @@ public static PolicyInitConfig readFromConfigProperties(ConfigProperties config)
168169
try (InputStream in = Files.newInputStream(Paths.get(mappingPathJson.trim()))) {
169170
return JsonPolicyInitConfigReader.read(in);
170171
} catch (IOException | RuntimeException e) {
171-
logger.log(
172-
Level.WARNING,
173-
"Failed to load telemetry policy init config from {0}",
174-
mappingPathJson.trim());
175-
logger.log(Level.WARNING, "Policy init config read failed", e);
172+
logReadFailure(mappingPathJson.trim(), e);
176173
return null;
177174
}
178175
}
179176
} else {
180177
try (InputStream in = Files.newInputStream(Paths.get(mappingPathYaml.trim()))) {
181178
return YamlPolicyInitConfigReader.read(in);
182179
} catch (IOException | RuntimeException e) {
183-
logger.log(
184-
Level.WARNING,
185-
"Failed to load telemetry policy init config from {0}",
186-
mappingPathYaml.trim());
187-
logger.log(Level.WARNING, "Policy init config read failed", e);
180+
logReadFailure(mappingPathYaml.trim(), e);
188181
return null;
189182
}
190183
}
@@ -200,7 +193,8 @@ private static PolicySourceConfig parseDeclarativeSource(
200193
requireDeclarativeText(
201194
sourceConfig.getString(FORMAT_DECLARATIVE_KEY),
202195
"Each source must define string 'format'.");
203-
String location = sourceConfig.getString(LOCATION_DECLARATIVE_KEY);
196+
String location =
197+
normalizeOptionalDeclarativeText(sourceConfig.getString(LOCATION_DECLARATIVE_KEY));
204198

205199
List<DeclarativeConfigProperties> mappingConfigs =
206200
sourceConfig.getStructuredList(MAPPINGS_DECLARATIVE_KEY);
@@ -244,13 +238,29 @@ private static String requireDeclarativeText(@Nullable String value, String mess
244238
return trimmed;
245239
}
246240

241+
@Nullable
242+
private static String normalizeOptionalDeclarativeText(@Nullable String value) {
243+
if (value == null) {
244+
return null;
245+
}
246+
String trimmed = value.trim();
247+
return trimmed.isEmpty() ? null : trimmed;
248+
}
249+
247250
@Nullable
248251
static ConfigProvider getConfigProvider(@Nullable OpenTelemetry openTelemetry) {
249252
return openTelemetry instanceof ExtendedOpenTelemetry
250253
? ((ExtendedOpenTelemetry) openTelemetry).getConfigProvider()
251254
: null;
252255
}
253256

257+
private static void logReadFailure(String configuredPath, Throwable throwable) {
258+
logger.log(
259+
Level.WARNING,
260+
"Failed to load telemetry policy init config from " + configuredPath,
261+
throwable);
262+
}
263+
254264
private static DeclarativeConfigProperties getGeneralDeclarativeConfig(
255265
ConfigProvider configProvider) {
256266
// Prefer the general config accessor when available in the API/runtime.

‎dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/registry/json/JsonNodePolicyInitConfigParser.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import io.opentelemetry.contrib.dynamic.policy.source.SourceKind;
1515
import java.util.ArrayList;
1616
import java.util.List;
17+
import javax.annotation.Nullable;
1718

1819
/** Shared JsonNode-to-model parser for registry init configuration. */
1920
public final class JsonNodePolicyInitConfigParser {
@@ -45,7 +46,9 @@ private static PolicySourceConfig parseSource(JsonNode node) {
4546

4647
JsonNode locationNode = objectNode.get("location");
4748
String location =
48-
locationNode != null && locationNode.isTextual() ? locationNode.asText() : null;
49+
locationNode != null && locationNode.isTextual()
50+
? normalizeOptionalText(locationNode.asText())
51+
: null;
4952

5053
JsonNode mappingsNode =
5154
requireArray(objectNode.get("mappings"), "Each source must define a 'mappings' array.");
@@ -91,4 +94,10 @@ private static String requireText(JsonNode node, String message) {
9194
}
9295
return value;
9396
}
97+
98+
@Nullable
99+
private static String normalizeOptionalText(String value) {
100+
String trimmed = value.trim();
101+
return trimmed.isEmpty() ? null : trimmed;
102+
}
94103
}

‎dynamic-control/src/test/java/io/opentelemetry/contrib/dynamic/policy/registry/PolicyInitConfigTest.java‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package io.opentelemetry.contrib.dynamic.policy.registry;
77

88
import static org.assertj.core.api.Assertions.assertThat;
9+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
910
import static org.mockito.Mockito.mock;
1011
import static org.mockito.Mockito.when;
1112

@@ -120,6 +121,23 @@ void readFromConfigPropertiesReturnsNullOnParseFailure() throws Exception {
120121
assertThat(configFromPaths(null, badJson.toString())).isNull();
121122
}
122123

124+
@Test
125+
void readFromConfigPropertiesTrimsLocationFromJsonFile() throws Exception {
126+
Path jsonPath = tempDir.resolve("policy-init.json");
127+
Files.write(jsonPath, jsonWithLocation(" from-json ").getBytes(StandardCharsets.UTF_8));
128+
129+
ConfigProperties config = mock(ConfigProperties.class);
130+
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)).thenReturn(null);
131+
when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON))
132+
.thenReturn(jsonPath.toString());
133+
134+
PolicyInitConfig initConfig = PolicyInitConfig.readFromConfigProperties(config);
135+
136+
assertThat(initConfig).isNotNull();
137+
assertThat(initConfig.getSources()).hasSize(1);
138+
assertThat(initConfig.getSources().get(0).getLocation()).isEqualTo("from-json");
139+
}
140+
123141
@Test
124142
void readFromDeclarativeConfigPropertiesReturnsNullWhenTelemetryPolicyMissing() {
125143
DeclarativeConfigProperties root = mock(DeclarativeConfigProperties.class);
@@ -128,6 +146,34 @@ void readFromDeclarativeConfigPropertiesReturnsNullWhenTelemetryPolicyMissing()
128146
assertThat(PolicyInitConfig.readFromDeclarativeConfigProperties(root)).isNull();
129147
}
130148

149+
@Test
150+
void readFromDeclarativeConfigPropertiesThrowsWhenTelemetryPolicySourcesMissing() {
151+
DeclarativeConfigProperties root = mock(DeclarativeConfigProperties.class);
152+
DeclarativeConfigProperties telemetryPolicy = mock(DeclarativeConfigProperties.class);
153+
when(root.getStructured(PolicyInitConfig.TELEMETRY_POLICY_DECLARATIVE_KEY))
154+
.thenReturn(telemetryPolicy);
155+
when(telemetryPolicy.getStructuredList(PolicyInitConfig.SOURCES_DECLARATIVE_KEY))
156+
.thenReturn(null);
157+
158+
assertThatThrownBy(() -> PolicyInitConfig.readFromDeclarativeConfigProperties(root))
159+
.isInstanceOf(IllegalArgumentException.class)
160+
.hasMessageContaining("sources");
161+
}
162+
163+
@Test
164+
void readFromDeclarativeConfigPropertiesThrowsWhenTelemetryPolicySourcesEmpty() {
165+
DeclarativeConfigProperties root = mock(DeclarativeConfigProperties.class);
166+
DeclarativeConfigProperties telemetryPolicy = mock(DeclarativeConfigProperties.class);
167+
when(root.getStructured(PolicyInitConfig.TELEMETRY_POLICY_DECLARATIVE_KEY))
168+
.thenReturn(telemetryPolicy);
169+
when(telemetryPolicy.getStructuredList(PolicyInitConfig.SOURCES_DECLARATIVE_KEY))
170+
.thenReturn(Collections.emptyList());
171+
172+
assertThatThrownBy(() -> PolicyInitConfig.readFromDeclarativeConfigProperties(root))
173+
.isInstanceOf(IllegalArgumentException.class)
174+
.hasMessageContaining("sources");
175+
}
176+
131177
@Test
132178
void readFromDeclarativeConfigPropertiesReadsTelemetryPolicySources() {
133179
DeclarativeConfigProperties root = mock(DeclarativeConfigProperties.class);
@@ -148,6 +194,20 @@ void readFromDeclarativeConfigPropertiesReadsTelemetryPolicySources() {
148194
assertThat(source.getMappings().get(0).getPolicyType()).isEqualTo("trace_sampling_rate_policy");
149195
}
150196

197+
@Test
198+
void readFromDeclarativeConfigPropertiesTrimsLocation() {
199+
DeclarativeConfigProperties root = mock(DeclarativeConfigProperties.class);
200+
DeclarativeConfigProperties telemetryPolicy = telemetryPolicyConfig(" from-declarative ");
201+
when(root.getStructured(PolicyInitConfig.TELEMETRY_POLICY_DECLARATIVE_KEY))
202+
.thenReturn(telemetryPolicy);
203+
204+
PolicyInitConfig config = PolicyInitConfig.readFromDeclarativeConfigProperties(root);
205+
206+
assertThat(config).isNotNull();
207+
assertThat(config.getSources()).hasSize(1);
208+
assertThat(config.getSources().get(0).getLocation()).isEqualTo("from-declarative");
209+
}
210+
151211
@Test
152212
void readFromDeclarativeOrConfigPropertiesPrefersDeclarativeWhenPresent() throws Exception {
153213
Path jsonPath = tempDir.resolve("policy-init.json");

0 commit comments

Comments
 (0)