From 20c466aa09eec73ff45e9c92d23d18c57f88abd5 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Mon, 10 Feb 2025 11:18:11 -0600 Subject: [PATCH 1/2] lwc-events: guard against exception from extractValue Ensure that if the extractValue implementation for an event throws an exception, it will not impact processing other events. If the extract fails, then it will treat the value as `null`. --- .../lwc/events/AbstractLwcEventClient.scala | 2 +- .../atlas/lwc/events/DatapointConverter.scala | 6 ++--- .../atlas/lwc/events/DatapointEvent.scala | 2 +- .../netflix/atlas/lwc/events/LwcEvent.scala | 24 +++++++++++++++++-- .../lwc/events/LwcEventDurationSuite.scala | 8 +++---- .../atlas/lwc/events/LwcEventSuite.scala | 13 ++++++---- 6 files changed, 40 insertions(+), 15 deletions(-) diff --git a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/AbstractLwcEventClient.scala b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/AbstractLwcEventClient.scala index f694c8e85..0300c5661 100644 --- a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/AbstractLwcEventClient.scala +++ b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/AbstractLwcEventClient.scala @@ -67,7 +67,7 @@ abstract class AbstractLwcEventClient(clock: Clock) extends LwcEventClient { expr.dataExpr, clock, sub.step, - Some(event => expr.projectionKeys.map(event.extractValue)), + Some(event => expr.projectionKeys.map(event.extractValueSafe)), submit ) EventHandler( diff --git a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointConverter.scala b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointConverter.scala index 4ec16ee16..b096e6d13 100644 --- a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointConverter.scala +++ b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointConverter.scala @@ -91,8 +91,8 @@ private[events] object DatapointConverter { case Some(k) => tags.get("statistic") match { case Some("count") => _ => 1.0 - case Some("totalOfSquares") => event => squared(event.extractValue(k), event.value) - case _ => event => toDouble(event.extractValue(k), event.value) + case Some("totalOfSquares") => event => squared(event.extractValueSafe(k), event.value) + case _ => event => toDouble(event.extractValueSafe(k), event.value) } case None => event => toDouble(event.value, 1.0) @@ -339,7 +339,7 @@ private[events] object DatapointConverter { private def getRawValue(event: LwcEvent): Any = { params.tags.get("value") match { - case Some(k) => event.extractValue(k) + case Some(k) => event.extractValueSafe(k) case None => event.value } } diff --git a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointEvent.scala b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointEvent.scala index a4d7a98f3..fa7639a20 100644 --- a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointEvent.scala +++ b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/DatapointEvent.scala @@ -59,7 +59,7 @@ case class DatapointEvent( @scala.annotation.tailrec private def encodeColumns(columns: List[String], gen: JsonGenerator): Unit = { if (columns.nonEmpty) { - Json.encode(gen, extractValue(columns.head)) + Json.encode(gen, extractValueSafe(columns.head)) encodeColumns(columns.tail, gen) } } diff --git a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala index c081f08b0..a2fdc59fc 100644 --- a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala +++ b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala @@ -17,6 +17,8 @@ package com.netflix.atlas.lwc.events import com.fasterxml.jackson.core.JsonGenerator import com.netflix.atlas.json.Json +import com.netflix.spectator.api.Spectator +import org.slf4j.LoggerFactory import java.io.StringWriter import scala.util.Using @@ -48,7 +50,7 @@ trait LwcEvent { * to ensure the two are consistent. */ def tagValue(key: String): String = { - extractValue(key) match { + extractValueSafe(key) match { case v: String => v case e: Enum[_] => e.name() case _ => null @@ -61,6 +63,24 @@ trait LwcEvent { */ def extractValue(key: String): Any + /** + * Internal method for extracting the value that handles exceptions if any. If an exception + * is thrown, then the value will be treated as `null`. The underlying exception will be + * logged at trace level as it can be quite noisy if it is a common pattern across events. + */ + private[events] final def extractValueSafe(key: String): Any = { + try { + extractValue(key) + } catch { + case e: Exception => + LoggerFactory.getLogger(getClass).trace(s"failed to extract value for key: $key", e) + Spectator.globalRegistry() + .counter("lwc.extractValueFailures", "error", e.getClass.getSimpleName) + .increment() + null + } + } + /** Encode the raw event as JSON. */ def encode(gen: JsonGenerator): Unit = { Json.encode(gen, rawEvent) @@ -78,7 +98,7 @@ trait LwcEvent { def encodeAsRow(columns: List[String], gen: JsonGenerator): Unit = { gen.writeStartArray() columns.foreach { key => - Json.encode(gen, extractValue(key)) + Json.encode(gen, extractValueSafe(key)) } gen.writeEndArray() } diff --git a/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventDurationSuite.scala b/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventDurationSuite.scala index 1f7d0ca89..30e0d8123 100644 --- a/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventDurationSuite.scala +++ b/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventDurationSuite.scala @@ -49,13 +49,13 @@ class LwcEventDurationSuite extends FunSuite { } test("extractValue: exists") { - assertEquals(sampleLwcEvent.extractValue("app"), "www") - assertEquals(sampleLwcEvent.extractValue("node"), "i-123") - assertEquals(sampleLwcEvent.extractValue("duration"), Duration.ofMillis(42)) + assertEquals(sampleLwcEvent.extractValueSafe("app"), "www") + assertEquals(sampleLwcEvent.extractValueSafe("node"), "i-123") + assertEquals(sampleLwcEvent.extractValueSafe("duration"), Duration.ofMillis(42)) } test("extractValue: missing") { - assertEquals(sampleLwcEvent.extractValue("foo"), null) + assertEquals(sampleLwcEvent.extractValueSafe("foo"), null) } test("defautl value") { diff --git a/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventSuite.scala b/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventSuite.scala index 586a36679..fc42e2216 100644 --- a/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventSuite.scala +++ b/atlas-lwc-events/src/test/scala/com/netflix/atlas/lwc/events/LwcEventSuite.scala @@ -48,13 +48,17 @@ class LwcEventSuite extends FunSuite { } test("extractValue: exists") { - assertEquals(sampleLwcEvent.extractValue("app"), "www") - assertEquals(sampleLwcEvent.extractValue("node"), "i-123") - assertEquals(sampleLwcEvent.extractValue("duration"), 42L) + assertEquals(sampleLwcEvent.extractValueSafe("app"), "www") + assertEquals(sampleLwcEvent.extractValueSafe("node"), "i-123") + assertEquals(sampleLwcEvent.extractValueSafe("duration"), 42L) } test("extractValue: missing") { - assertEquals(sampleLwcEvent.extractValue("foo"), null) + assertEquals(sampleLwcEvent.extractValueSafe("foo"), null) + } + + test("extractValue: exception thrown") { + assertEquals(sampleLwcEvent.extractValueSafe("npe"), null) } test("toJson: raw event") { @@ -87,6 +91,7 @@ object LwcEventSuite { case "tags" => span.tags case "duration" => span.duration case "level" => Logger.Level.TRACE + case "npe" => throw new NullPointerException() case k => span.tags.getOrElse(k, null) } } From a151e275b476b0d1bb71200f1c7ea0922388f343 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Mon, 10 Feb 2025 11:22:57 -0600 Subject: [PATCH 2/2] wip --- .../src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala index a2fdc59fc..3a40c0796 100644 --- a/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala +++ b/atlas-lwc-events/src/main/scala/com/netflix/atlas/lwc/events/LwcEvent.scala @@ -74,7 +74,8 @@ trait LwcEvent { } catch { case e: Exception => LoggerFactory.getLogger(getClass).trace(s"failed to extract value for key: $key", e) - Spectator.globalRegistry() + Spectator + .globalRegistry() .counter("lwc.extractValueFailures", "error", e.getClass.getSimpleName) .increment() null