Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lwc-events: guard against exception from extractValue #1745

Merged
merged 2 commits into from
Feb 10, 2025
Merged
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 @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -61,6 +63,25 @@ 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)
Expand All @@ -78,7 +99,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()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down Expand Up @@ -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)
}
}
Expand Down