From e9626a524b404bf65bd75c71f91a3ac7cb24fa45 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Thu, 30 May 2024 12:41:35 +0200 Subject: [PATCH] Report exception when deserializing config Send probe diagnostic messages when an error occurred during config deserialization (compatibility issue, unsupported feature, malformed json, parsing bug) --- .../debugger/agent/ConfigurationAcceptor.java | 2 + .../debugger/agent/ConfigurationUpdater.java | 31 ++++++++++ .../agent/DebuggerProductChangesListener.java | 57 +++++++++++-------- .../agent/ConfigurationUpdaterTest.java | 9 +++ .../DebuggerProductChangesListenerTest.java | 30 ++++++++++ 5 files changed, 105 insertions(+), 24 deletions(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationAcceptor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationAcceptor.java index e11623c7a06..a69a5e5b44b 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationAcceptor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationAcceptor.java @@ -10,4 +10,6 @@ enum Source { } void accept(Source source, Collection definitions); + + void handleException(String configId, Exception ex); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java index d08c8e3c9c4..24a1c51916f 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java @@ -1,5 +1,9 @@ package com.datadog.debugger.agent; +import static com.datadog.debugger.agent.DebuggerProductChangesListener.LOG_PROBE_PREFIX; +import static com.datadog.debugger.agent.DebuggerProductChangesListener.METRIC_PROBE_PREFIX; +import static com.datadog.debugger.agent.DebuggerProductChangesListener.SPAN_DECORATION_PROBE_PREFIX; +import static com.datadog.debugger.agent.DebuggerProductChangesListener.SPAN_PROBE_PREFIX; import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY; import com.datadog.debugger.instrumentation.InstrumentationResult; @@ -9,6 +13,7 @@ import com.datadog.debugger.util.ExceptionHelper; import datadog.trace.api.Config; import datadog.trace.bootstrap.debugger.DebuggerContext; +import datadog.trace.bootstrap.debugger.ProbeId; import datadog.trace.bootstrap.debugger.ProbeImplementation; import datadog.trace.bootstrap.debugger.ProbeRateLimiter; import datadog.trace.util.TagsHelper; @@ -72,6 +77,7 @@ public ConfigurationUpdater( // /!\ Can be called by different threads and concurrently /!\ // Should throw a runtime exception if there is a problem. The message of // the exception will be reported in the next request to the conf service + @Override public void accept(Source source, Collection definitions) { try { LOGGER.debug("Received new definitions from {}", source); @@ -84,6 +90,31 @@ public void accept(Source source, Collection definiti } } + @Override + public void handleException(String configId, Exception ex) { + if (configId == null) { + return; + } + ProbeId probeId; + if (configId.startsWith(LOG_PROBE_PREFIX)) { + probeId = extractPrefix(LOG_PROBE_PREFIX, configId); + } else if (configId.startsWith(METRIC_PROBE_PREFIX)) { + probeId = extractPrefix(METRIC_PROBE_PREFIX, configId); + } else if (configId.startsWith(SPAN_PROBE_PREFIX)) { + probeId = extractPrefix(SPAN_PROBE_PREFIX, configId); + } else if (configId.startsWith(SPAN_DECORATION_PROBE_PREFIX)) { + probeId = extractPrefix(SPAN_DECORATION_PROBE_PREFIX, configId); + } else { + probeId = new ProbeId(configId, 0); + } + LOGGER.warn("Error handling probe configuration: {}", configId, ex); + sink.getProbeStatusSink().addError(probeId, ex); + } + + private ProbeId extractPrefix(String prefix, String configId) { + return new ProbeId(configId.substring(prefix.length()), 0); + } + private void applyNewConfiguration(Configuration newConfiguration) { configurationLock.lock(); try { diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerProductChangesListener.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerProductChangesListener.java index 26ed9154156..fc653af3526 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerProductChangesListener.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerProductChangesListener.java @@ -32,6 +32,10 @@ public class DebuggerProductChangesListener implements ProductListener { public static final int MAX_ALLOWED_LOG_PROBES = 100; public static final int MAX_ALLOWED_SPAN_PROBES = 100; public static final int MAX_ALLOWED_SPAN_DECORATION_PROBES = 100; + public static final String LOG_PROBE_PREFIX = "logProbe_"; + public static final String METRIC_PROBE_PREFIX = "metricProbe_"; + public static final String SPAN_PROBE_PREFIX = "spanProbe_"; + public static final String SPAN_DECORATION_PROBE_PREFIX = "spanDecorationProbe_"; private static final Logger LOGGER = LoggerFactory.getLogger(DebuggerProductChangesListener.class); @@ -97,32 +101,37 @@ public void accept( datadog.remoteconfig.ConfigurationChangesListener.PollingRateHinter pollingRateHinter) throws IOException { String configId = configKey.getConfigId(); - if (configId.startsWith("metricProbe_")) { - MetricProbe metricProbe = Adapter.deserializeMetricProbe(content); - configChunks.put(configId, definitions -> definitions.add(metricProbe)); - } else if (configId.startsWith("logProbe_")) { - LogProbe logProbe = Adapter.deserializeLogProbe(content); - configChunks.put(configId, definitions -> definitions.add(logProbe)); - } else if (configId.startsWith("spanProbe_")) { - SpanProbe spanProbe = Adapter.deserializeSpanProbe(content); - configChunks.put(configId, definitions -> definitions.add(spanProbe)); - } else if (configId.startsWith("spanDecorationProbe_")) { - SpanDecorationProbe spanDecorationProbe = Adapter.deserializeSpanDecorationProbe(content); - configChunks.put(configId, definitions -> definitions.add(spanDecorationProbe)); - } else if (IS_UUID.test(configId)) { - Configuration newConfig = Adapter.deserializeConfiguration(content); - if (newConfig.getService().equals(serviceName)) { - configChunks.put( - configId, - (builder) -> { - builder.addAll(newConfig.getDefinitions()); - }); + try { + if (configId.startsWith(METRIC_PROBE_PREFIX)) { + MetricProbe metricProbe = Adapter.deserializeMetricProbe(content); + configChunks.put(configId, definitions -> definitions.add(metricProbe)); + } else if (configId.startsWith(LOG_PROBE_PREFIX)) { + LogProbe logProbe = Adapter.deserializeLogProbe(content); + configChunks.put(configId, definitions -> definitions.add(logProbe)); + } else if (configId.startsWith(SPAN_PROBE_PREFIX)) { + SpanProbe spanProbe = Adapter.deserializeSpanProbe(content); + configChunks.put(configId, definitions -> definitions.add(spanProbe)); + } else if (configId.startsWith(SPAN_DECORATION_PROBE_PREFIX)) { + SpanDecorationProbe spanDecorationProbe = Adapter.deserializeSpanDecorationProbe(content); + configChunks.put(configId, definitions -> definitions.add(spanDecorationProbe)); + } else if (IS_UUID.test(configId)) { + Configuration newConfig = Adapter.deserializeConfiguration(content); + if (newConfig.getService().equals(serviceName)) { + configChunks.put( + configId, + (builder) -> { + builder.addAll(newConfig.getDefinitions()); + }); + } else { + throw new IOException( + "got config.serviceName = " + newConfig.getService() + ", ignoring configuration"); + } } else { - throw new IOException( - "got config.serviceName = " + newConfig.getService() + ", ignoring configuration"); + LOGGER.debug("Unsupported configuration id: {}, ignoring configuration", configId); } - } else { - LOGGER.debug("Unsupported configuration id: {}, ignoring configuration", configId); + } catch (Exception ex) { + configurationAcceptor.handleException(configId, ex); + throw new IOException(ex); } } diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java index 6268c9e8b21..b2fe9123d6f 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java @@ -1,6 +1,7 @@ package com.datadog.debugger.agent; import static com.datadog.debugger.agent.ConfigurationAcceptor.Source.REMOTE_CONFIG; +import static com.datadog.debugger.agent.DebuggerProductChangesListener.LOG_PROBE_PREFIX; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -614,6 +615,14 @@ public void acceptNewDecorationSpan() { assertTrue(appliedDefinitions.containsKey(SPAN_DECORATION_ID.getEncodedId())); } + @Test + public void handleException() { + ConfigurationUpdater configurationUpdater = createConfigUpdater(debuggerSinkWithMockStatusSink); + Exception ex = new Exception("oops"); + configurationUpdater.handleException(LOG_PROBE_PREFIX + PROBE_ID.getId(), ex); + verify(probeStatusSink).addError(eq(ProbeId.from(PROBE_ID.getId() + ":0")), eq(ex)); + } + private DebuggerTransformer createTransformer( Config tracerConfig, Configuration configuration, diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/DebuggerProductChangesListenerTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/DebuggerProductChangesListenerTest.java index f63fb3eaccd..a65fbf70c73 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/DebuggerProductChangesListenerTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/DebuggerProductChangesListenerTest.java @@ -7,7 +7,9 @@ import static com.datadog.debugger.util.LogProbeTestHelper.parseTemplate; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.lenient; @@ -44,15 +46,25 @@ public class DebuggerProductChangesListenerTest { class SimpleAcceptor implements ConfigurationAcceptor { private Collection definitions; + private Exception lastException; @Override public void accept(Source source, Collection definitions) { this.definitions = definitions; } + @Override + public void handleException(String configId, Exception ex) { + lastException = ex; + } + public Collection getDefinitions() { return definitions; } + + public Exception getLastException() { + return lastException; + } } @BeforeEach @@ -256,6 +268,24 @@ public void maxSpanDecorationProbes() { assertEquals(MAX_ALLOWED_SPAN_DECORATION_PROBES, acceptor.getDefinitions().size()); } + @Test + public void parsingException() throws IOException { + SimpleAcceptor acceptor = new SimpleAcceptor(); + DebuggerProductChangesListener listener = + new DebuggerProductChangesListener(tracerConfig, acceptor); + String probeUUID = UUID.randomUUID().toString(); + IOException ioException = + assertThrows( + IOException.class, + () -> + listener.accept( + createConfigKey("logProbe_" + probeUUID), + "{bad json}".getBytes(StandardCharsets.UTF_8), + pollingHinter)); + assertNotNull(acceptor.lastException); + assertEquals(ioException.getCause(), acceptor.lastException); + } + private void assertDefinitions( Collection actualDefinitions, ProbeDefinition... expectedDefinitions) {