Skip to content

Commit dfd4bba

Browse files
committed
Address PR feedback
- Rename 'emf' config property to 'emf_metrics' for clarity - Improve test with random keys/values and proper verification using MetricsContext - Remove unused DEFAULT constant from EMFLoggingRegistryConfig Signed-off-by: Manuel Mangas Zurita <mzurita@amazon.com>
1 parent 89e7833 commit dfd4bba

4 files changed

Lines changed: 24 additions & 10 deletions

File tree

data-prepper-core/src/main/java/org/opensearch/dataprepper/core/meter/EMFLoggingRegistryConfig.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import java.util.Map;
1212

1313
public interface EMFLoggingRegistryConfig extends StepRegistryConfig {
14-
EMFLoggingRegistryConfig DEFAULT = k -> null;
1514

1615
@Override
1716
default String prefix() {

data-prepper-core/src/main/java/org/opensearch/dataprepper/core/parser/model/DataPrepperConfiguration.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public DataPrepperConfiguration(
9494
final List<MetricTagFilter> metricTagFilters,
9595
@JsonProperty("disabled_metrics")
9696
final List<String> disabledMetrics,
97-
@JsonProperty("emf")
97+
@JsonProperty("emf_metrics")
9898
final EmfConfig emf,
9999
@JsonProperty("peer_forwarder") final PeerForwarderConfiguration peerForwarderConfiguration,
100100
@JsonProperty("processor_shutdown_timeout")

data-prepper-core/src/test/java/org/opensearch/dataprepper/core/meter/EMFLoggingMeterRegistryTest.java

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import software.amazon.cloudwatchlogs.emf.model.Unit;
2929

3030
import java.lang.reflect.Field;
31+
import java.time.Duration;
3132
import java.util.Arrays;
3233
import java.util.Collections;
3334
import java.util.List;
@@ -60,7 +61,7 @@ class EMFLoggingMeterRegistryTest {
6061
private final MockClock clock = new MockClock();
6162
private final EMFLoggingMeterRegistry registry = spy(
6263
new EMFLoggingMeterRegistry(
63-
EMFLoggingRegistryConfig.DEFAULT, new EnvironmentProvider().resolveEnvironment().join(), clock)
64+
k -> null, new EnvironmentProvider().resolveEnvironment().join(), clock)
6465
);
6566
private final EMFLoggingMeterRegistry.Snapshot registrySnapshot = registry.new Snapshot();
6667

@@ -218,7 +219,7 @@ void snapshotFunctionCounterDataValid() {
218219
void snapshotFunctionCounterDataShouldClampInfiniteValues() {
219220
FunctionCounter functionCounter = FunctionCounter
220221
.builder("my.positive.infinity", Double.POSITIVE_INFINITY, Number::doubleValue).register(registry);
221-
clock.add(EMFLoggingRegistryConfig.DEFAULT.step());
222+
clock.add(Duration.ofMinutes(1));
222223
final List<EMFLoggingMeterRegistry.MetricDataPoint> metricDataPoints1 = registrySnapshot
223224
.functionCounterData(functionCounter)
224225
.collect(Collectors.toList());
@@ -227,7 +228,7 @@ void snapshotFunctionCounterDataShouldClampInfiniteValues() {
227228

228229
functionCounter = FunctionCounter
229230
.builder("my.negative.infinity", Double.NEGATIVE_INFINITY, Number::doubleValue).register(registry);
230-
clock.add(EMFLoggingRegistryConfig.DEFAULT.step());
231+
clock.add(Duration.ofMinutes(1));
231232
final List<EMFLoggingMeterRegistry.MetricDataPoint> metricDataPoints2 = registrySnapshot
232233
.functionCounterData(functionCounter)
233234
.collect(Collectors.toList());
@@ -270,7 +271,7 @@ void snapshotShouldNotAddFunctionTimerDataWhenSumIsNaN() {
270271
final FunctionTimer functionTimer = FunctionTimer
271272
.builder("my.function.timer", Double.NaN, Number::longValue, Number::doubleValue, TimeUnit.MILLISECONDS)
272273
.register(registry);
273-
clock.add(EMFLoggingRegistryConfig.DEFAULT.step());
274+
clock.add(Duration.ofMinutes(1));
274275
final List<EMFLoggingMeterRegistry.MetricDataPoint> metricDataPoints = registrySnapshot
275276
.functionTimerData(functionTimer)
276277
.collect(Collectors.toList());
@@ -358,7 +359,12 @@ private MetricsContext reflectivelyGetMetricsContext(final MetricsLogger metrics
358359

359360
@Test
360361
void testAdditionalPropertiesAreAddedToMetricsLogger() {
361-
final Map<String, String> additionalProperties = Map.of("key1", "value1", "key2", "value2");
362+
final String randomKey1 = "testKey_" + System.currentTimeMillis();
363+
final String randomValue1 = "testValue_" + Math.random();
364+
final String randomKey2 = "anotherKey_" + System.nanoTime();
365+
final String randomValue2 = "anotherValue_" + Math.random();
366+
367+
final Map<String, String> additionalProperties = Map.of(randomKey1, randomValue1, randomKey2, randomValue2);
362368
final EMFLoggingRegistryConfig config = new EMFLoggingRegistryConfig() {
363369
@Override
364370
public String get(String key) {
@@ -378,8 +384,17 @@ public Map<String, String> additionalProperties() {
378384
final List<MetricsLogger> metricsLoggers = registryWithProperties.metricsLoggers();
379385

380386
assertThat(metricsLoggers.size(), equalTo(1));
381-
// Note: We can't directly verify putProperty calls without mocking the MetricsLogger
382-
// This test verifies the registry can be created with additional properties config
387+
388+
final MetricsLogger metricsLogger = metricsLoggers.get(0);
389+
final MetricsContext context = reflectivelyGetMetricsContext(metricsLogger);
390+
391+
// Verify properties exist
392+
assertThat(context.getProperty(randomKey1), equalTo(randomValue1));
393+
assertThat(context.getProperty(randomKey2), equalTo(randomValue2));
394+
395+
// Verify properties are not dimensions
396+
assertThat(hasDimension(context, randomKey1), is(false));
397+
assertThat(hasDimension(context, randomKey2), is(false));
383398
}
384399

385400
private boolean hasDimension(final MetricsContext context, final String key) {

data-prepper-core/src/test/java/org/opensearch/dataprepper/core/meter/EMFLoggingRegistryConfigTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
class EMFLoggingRegistryConfigTest {
1414
@Test
1515
void testDefault() {
16-
final EMFLoggingRegistryConfig objectUnderTest = EMFLoggingRegistryConfig.DEFAULT;
16+
final EMFLoggingRegistryConfig objectUnderTest = k -> null;
1717
assertThat(objectUnderTest.prefix(), equalTo("emf"));
1818
}
1919
}

0 commit comments

Comments
 (0)