Skip to content

Commit 838a8e3

Browse files
bidetofevilbidetofevil
authored
Move span event validation so truncation (#2404)
## Goal Events were dropped if they had names that were too long or if they contained too many attributes. Attributes were dropped if their keys or values were too long. This adds similar truncation logic, and moves the check out of the `EmbraceSpanEvent` factory method, which was a terrible idea to begin with. Now, it basically just creates the object and allows the truncation to happen at a lower layer. In addition, the span name used by the created span is now wholly inside `EmbraceSpanImpl`. This is so that the arguments object's name can be immutable so truncation can be localized inside the `EmbraceSdkSpan` implementation.
1 parent 24a7e74 commit 838a8e3

File tree

12 files changed

+140
-79
lines changed

12 files changed

+140
-79
lines changed

‎embrace-android-api/src/main/kotlin/io/embrace/android/embracesdk/spans/EmbraceSpanEvent.kt‎

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,25 +26,15 @@ public data class EmbraceSpanEvent internal constructor(
2626
* @suppress
2727
*/
2828
public companion object {
29-
internal const val MAX_EVENT_NAME_LENGTH = 100
30-
internal const val MAX_EVENT_ATTRIBUTE_COUNT = 10
31-
3229
/**
3330
* @suppress
3431
*/
3532
public fun create(name: String, timestampMs: Long, attributes: Map<String, String>?): EmbraceSpanEvent? {
36-
if (inputsValid(name, attributes)) {
37-
return EmbraceSpanEvent(
38-
name = name,
39-
timestampNanos = TimeUnit.MILLISECONDS.toNanos(timestampMs),
40-
attributes = attributes ?: emptyMap()
41-
)
42-
}
43-
44-
return null
33+
return EmbraceSpanEvent(
34+
name = name,
35+
timestampNanos = TimeUnit.MILLISECONDS.toNanos(timestampMs),
36+
attributes = attributes ?: emptyMap()
37+
)
4538
}
46-
47-
internal fun inputsValid(name: String, attributes: Map<String, String>?) =
48-
name.length <= MAX_EVENT_NAME_LENGTH && (attributes == null || attributes.size <= MAX_EVENT_ATTRIBUTE_COUNT)
4939
}
5040
}

‎embrace-android-otel/src/main/kotlin/io/embrace/android/embracesdk/internal/otel/sdk/DataValidator.kt‎

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ class DataValidator(
1212
val otelLimitsConfig: OtelLimitsConfig = OtelLimitsConfigImpl,
1313
private val bypassValidation: (() -> Boolean) = { false },
1414
) {
15-
fun truncateSpanName(name: String, internal: Boolean): String {
15+
fun truncateName(name: String, internal: Boolean): String {
1616
val maxLength = if (internal) {
1717
otelLimitsConfig.getMaxInternalNameLength()
1818
} else {
@@ -31,8 +31,8 @@ class DataValidator(
3131
}
3232
}
3333

34-
fun truncateAttributes(attributes: Map<String, String>, internal: Boolean): Map<String, String> {
35-
val maxAttributeCount = if (internal) {
34+
fun truncateAttributes(attributes: Map<String, String>, internal: Boolean, countOverride: Int? = null): Map<String, String> {
35+
val maxAttributeCount = countOverride ?: if (internal) {
3636
otelLimitsConfig.getMaxSystemAttributeCount()
3737
} else {
3838
otelLimitsConfig.getMaxCustomAttributeCount()
@@ -80,19 +80,43 @@ class DataValidator(
8080
return Pair(PropertyUtils.truncate(key, maxKeyLength), truncatedValue)
8181
}
8282

83+
fun createTruncatedSpanEvent(
84+
name: String,
85+
timestampMs: Long,
86+
internal: Boolean,
87+
attributes: Map<String, String>,
88+
): EmbraceSpanEvent? {
89+
return EmbraceSpanEvent.create(
90+
name = truncateName(name, internal),
91+
timestampMs = timestampMs,
92+
attributes = truncateAttributes(
93+
attributes = attributes,
94+
internal = internal,
95+
countOverride = otelLimitsConfig.getMaxEventAttributeCount()
96+
)
97+
)
98+
}
99+
83100
private fun Map<String, String>.truncate(
84101
maxCount: Int,
85102
maxKeyLength: Int,
86103
maxValueLength: Int,
87104
): Map<String, String> =
88-
mutableMapOf<String, String>().apply {
89-
this@truncate.entries.take(maxCount).forEach {
90-
val truncatedValue = if (it.key.isValidLongValueAttribute()) {
91-
it.value
92-
} else {
93-
PropertyUtils.truncate(it.value, maxValueLength)
94-
}
95-
this[PropertyUtils.truncate(it.key, maxKeyLength)] = truncatedValue
96-
}
105+
entries.take(maxCount).associate {
106+
PropertyUtils.truncate(
107+
value = it.key,
108+
maxLength = maxKeyLength
109+
) to truncateAttributeValue(
110+
key = it.key,
111+
value = it.value,
112+
maxLength = maxValueLength
113+
)
114+
}
115+
116+
private fun truncateAttributeValue(key: String, value: String, maxLength: Int): String =
117+
if (key.isValidLongValueAttribute()) {
118+
value
119+
} else {
120+
PropertyUtils.truncate(value, maxLength)
97121
}
98122
}

‎embrace-android-otel/src/main/kotlin/io/embrace/android/embracesdk/internal/otel/spans/EmbraceSpanFactoryImpl.kt‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,10 @@ private class EmbraceSpanImpl(
113113

114114
@Volatile
115115
override var status = io.embrace.android.embracesdk.internal.payload.Span.Status.UNSET
116-
private var updatedName: String? = null
116+
private var spanName: String = validateName(otelSpanCreator.spanStartArgs.initialSpanName)
117+
set(name) {
118+
field = validateName(name)
119+
}
117120

118121
private val systemEvents = ConcurrentLinkedQueue<EmbraceSpanEvent>()
119122
private val customEvents = ConcurrentLinkedQueue<EmbraceSpanEvent>()
@@ -168,9 +171,8 @@ private class EmbraceSpanImpl(
168171
}
169172

170173
spanRepository.trackStartedSpan(this)
171-
updatedName?.let { newName ->
172-
newSpan.name = newName
173-
}
174+
newSpan.name = spanName
175+
174176
spanStartTimeMs = attemptedStartTimeMs
175177
spanRepository.notifySpanUpdate()
176178
}
@@ -224,10 +226,11 @@ private class EmbraceSpanImpl(
224226

225227
override fun addEvent(name: String, timestampMs: Long?, attributes: Map<String, String>?): Boolean =
226228
addObject(customEvents, customEventCount, dataValidator.otelLimitsConfig.getMaxCustomEventCount()) {
227-
EmbraceSpanEvent.create(
229+
dataValidator.createTruncatedSpanEvent(
228230
name = name,
229231
timestampMs = timestampMs?.normalizeTimestampAsMillis() ?: openTelemetryClock.now().nanosToMillis(),
230-
attributes = attributes
232+
internal = otelSpanCreator.spanStartArgs.internal,
233+
attributes = attributes ?: emptyMap(),
231234
)
232235
}
233236

@@ -248,19 +251,21 @@ private class EmbraceSpanImpl(
248251

249252
eventAttributes[ExceptionAttributes.EXCEPTION_STACKTRACE.key] = exception.truncatedStacktraceText()
250253

251-
EmbraceSpanEvent.create(
254+
dataValidator.createTruncatedSpanEvent(
252255
name = dataValidator.otelLimitsConfig.getExceptionEventName(),
253256
timestampMs = openTelemetryClock.now().nanosToMillis(),
254-
attributes = eventAttributes
257+
internal = otelSpanCreator.spanStartArgs.internal,
258+
attributes = eventAttributes,
255259
)
256260
}
257261

258262
override fun addSystemEvent(name: String, timestampMs: Long?, attributes: Map<String, String>?): Boolean =
259263
addObject(systemEvents, systemEventCount, dataValidator.otelLimitsConfig.getMaxSystemEventCount()) {
260-
EmbraceSpanEvent.create(
264+
dataValidator.createTruncatedSpanEvent(
261265
name = name,
262266
timestampMs = timestampMs?.normalizeTimestampAsMillis() ?: openTelemetryClock.now().nanosToMillis(),
263-
attributes = attributes
267+
internal = otelSpanCreator.spanStartArgs.internal,
268+
attributes = attributes ?: emptyMap(),
264269
)
265270
}
266271

@@ -313,9 +318,8 @@ private class EmbraceSpanImpl(
313318
if (newName.isNotBlank()) {
314319
synchronized(startedSpan) {
315320
if (!spanStarted() || isRecording) {
316-
val validatedName = dataValidator.truncateSpanName(newName, otelSpanCreator.spanStartArgs.internal)
317-
updatedName = validatedName
318-
startedSpan.get()?.name = validatedName
321+
spanName = newName
322+
startedSpan.get()?.name = spanName
319323
spanRepository.notifySpanUpdate()
320324
return true
321325
}
@@ -399,7 +403,7 @@ private class EmbraceSpanImpl(
399403
}
400404

401405
override fun name(): String = synchronized(startedSpan) {
402-
updatedName ?: otelSpanCreator.spanStartArgs.spanName
406+
spanName
403407
}
404408

405409
override val spanKind: SpanKind
@@ -497,4 +501,10 @@ private class EmbraceSpanImpl(
497501
}
498502
}
499503
}
504+
505+
private fun validateName(name: String) =
506+
dataValidator.truncateName(
507+
name = name,
508+
internal = otelSpanCreator.spanStartArgs.internal
509+
)
500510
}

‎embrace-android-otel/src/main/kotlin/io/embrace/android/embracesdk/internal/otel/spans/OtelSpanCreator.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ class OtelSpanCreator(
1616
internal fun startSpan(startTimeMs: Long): Span {
1717
val parentSpanContext = spanStartArgs.getParentSpanContext()
1818
return tracer.createSpan(
19-
name = spanStartArgs.spanName,
19+
name = spanStartArgs.initialSpanName,
2020
parent = parentSpanContext?.let(::SpanContextAdapter),
2121
spanKind = spanStartArgs.spanKind?.convertToOtelKotlin() ?: SpanKind.INTERNAL,
2222
startTimestamp = startTimeMs.millisToNanos()

‎embrace-android-otel/src/main/kotlin/io/embrace/android/embracesdk/internal/otel/spans/OtelSpanStartArgs.kt‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,7 @@ class OtelSpanStartArgs(
2323
parentSpan: EmbraceSpan? = null,
2424
) {
2525
var parentContext: OtelJavaContext = OtelJavaContext.root()
26-
27-
var spanName: String = name.appendEmbracePrefix(internal)
28-
set(value) {
29-
field = value.appendEmbracePrefix(internal)
30-
}
31-
26+
val initialSpanName: String = name.prependEmbracePrefix(internal)
3227
var startTimeMs: Long? = null
3328
var spanKind: OtelJavaSpanKind? = null
3429

@@ -59,7 +54,7 @@ class OtelSpanStartArgs(
5954
}
6055
}
6156

62-
private fun String.appendEmbracePrefix(internal: Boolean): String {
57+
private fun String.prependEmbracePrefix(internal: Boolean): String {
6358
return if (internal) {
6459
toEmbraceObjectName()
6560
} else {

‎embrace-android-otel/src/main/kotlin/io/embrace/android/embracesdk/internal/otel/spans/SpanServiceImpl.kt‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class SpanServiceImpl(
4242
EmbTrace.trace("span-create") {
4343
return if (name.isNotBlank() && canStartNewSpan(parent, internal)) {
4444
embraceSpanFactory.create(
45-
name = dataValidator.truncateSpanName(name, internal),
45+
name = dataValidator.truncateName(name, internal),
4646
type = type,
4747
internal = internal,
4848
private = private,
@@ -57,11 +57,9 @@ class SpanServiceImpl(
5757

5858
override fun createSpan(otelSpanCreator: OtelSpanCreator): EmbraceSdkSpan? {
5959
EmbTrace.trace("span-create") {
60-
val otelSpanStartArgs = otelSpanCreator.spanStartArgs.apply {
61-
spanName = dataValidator.truncateSpanName(spanName, internal)
62-
}
60+
val otelSpanStartArgs = otelSpanCreator.spanStartArgs
6361
return if (
64-
otelSpanStartArgs.spanName.isNotBlank() &&
62+
otelSpanStartArgs.initialSpanName.isNotBlank() &&
6563
canStartNewSpan(
6664
otelSpanStartArgs.parentContext.getEmbraceSpan(),
6765
otelSpanStartArgs.internal
@@ -135,7 +133,7 @@ class SpanServiceImpl(
135133
return false
136134
}
137135

138-
val validName = dataValidator.truncateSpanName(name, internal)
136+
val validName = dataValidator.truncateName(name, internal)
139137
val validEvents = dataValidator.truncateEvents(events, internal)
140138
val validAttributes = dataValidator.truncateAttributes(attributes, internal)
141139

‎embrace-android-otel/src/test/kotlin/io/embrace/android/embracesdk/internal/otel/spans/EmbraceSpanImplTest.kt‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import io.embrace.android.embracesdk.fixtures.TRUNCATED_TOO_LONG_ATTRIBUTE_KEY
2121
import io.embrace.android.embracesdk.fixtures.TRUNCATED_TOO_LONG_ATTRIBUTE_KEY_FOR_INTERNAL_SPAN
2222
import io.embrace.android.embracesdk.fixtures.TRUNCATED_TOO_LONG_ATTRIBUTE_VALUE
2323
import io.embrace.android.embracesdk.fixtures.TRUNCATED_TOO_LONG_ATTRIBUTE_VALUE_FOR_INTERNAL_SPAN
24+
import io.embrace.android.embracesdk.fixtures.TRUNCATED_TOO_LONG_EVENT_NAME
2425
import io.embrace.android.embracesdk.fixtures.TRUNCATED_TOO_LONG_SPAN_NAME
2526
import io.embrace.android.embracesdk.fixtures.fakeContextKey
2627
import io.embrace.android.embracesdk.fixtures.maxSizeEventAttributes
@@ -191,6 +192,7 @@ internal class EmbraceSpanImplTest {
191192
fun `check adding events`() {
192193
with(embraceSpan) {
193194
assertTrue(start())
195+
assertTrue(addEvent(name = ""))
194196
assertTrue(addEvent(name = "current event"))
195197
assertTrue(
196198
addEvent(
@@ -268,16 +270,17 @@ internal class EmbraceSpanImplTest {
268270
assertTrue(start())
269271
assertTrue(recordException(exception = firstException))
270272
assertTrue(recordException(exception = secondException, attributes = mapOf("myKey" to "myValue")))
271-
assertFalse(recordException(exception = RuntimeException(), attributes = tooBigEventAttributes))
273+
assertTrue(recordException(exception = RuntimeException(), attributes = tooBigEventAttributes))
272274
assertTrue(stop())
273275
assertFalse(recordException(exception = IllegalStateException()))
276+
assertEquals(3, events().size)
274277
assertTrue(updateNotified)
275278
}
276279

277280
with(checkNotNull(embraceSpan.snapshot())) {
278281
val sanitizedEvents = checkNotNull(events)
279-
assertEquals(2, sanitizedEvents.size)
280-
with(sanitizedEvents.first()) {
282+
assertEquals(3, sanitizedEvents.size)
283+
with(sanitizedEvents[0]) {
281284
assertEquals(dataValidator.otelLimitsConfig.getExceptionEventName(), name)
282285
val attrs = checkNotNull(attributes)
283286
assertEquals(timestampNanos, timestampNanos)
@@ -291,7 +294,7 @@ internal class EmbraceSpanImplTest {
291294
attrs.single { it.key == ExceptionAttributes.EXCEPTION_STACKTRACE.key }.data
292295
)
293296
}
294-
with(sanitizedEvents.last()) {
297+
with(sanitizedEvents[1]) {
295298
assertEquals(dataValidator.otelLimitsConfig.getExceptionEventName(), name)
296299
val attrs = checkNotNull(attributes)
297300
assertEquals(timestampNanos, timestampNanos)
@@ -324,14 +327,15 @@ internal class EmbraceSpanImplTest {
324327
fun `check event limits`() {
325328
with(embraceSpan) {
326329
assertTrue(start())
327-
assertFalse(addEvent(name = TOO_LONG_EVENT_NAME))
328-
assertFalse(addEvent(name = TOO_LONG_EVENT_NAME, timestampMs = null, attributes = null))
329-
assertFalse(addEvent(name = "yo", timestampMs = null, attributes = tooBigEventAttributes))
330+
assertTrue(addEvent(name = TOO_LONG_EVENT_NAME))
331+
assertEquals(TRUNCATED_TOO_LONG_EVENT_NAME, events().last().name)
332+
assertTrue(addEvent(name = "yo", timestampMs = null, attributes = tooBigEventAttributes))
333+
assertEquals(10, events().last().attributes?.size)
330334
assertTrue(addEvent(name = MAX_LENGTH_EVENT_NAME))
331335
assertTrue(addEvent(name = MAX_LENGTH_EVENT_NAME, timestampMs = null, attributes = null))
332336
assertTrue(addEvent(name = "yo", timestampMs = null, attributes = maxSizeEventAttributes))
333337
assertTrue(recordException(exception = RuntimeException()))
334-
repeat(dataValidator.otelLimitsConfig.getMaxCustomEventCount() - 5) {
338+
repeat(dataValidator.otelLimitsConfig.getMaxCustomEventCount() - 7) {
335339
assertTrue(addEvent(name = "event $it"))
336340
}
337341
val eventAttributesAMap = mutableMapOf(

‎embrace-android-otel/src/test/kotlin/io/embrace/android/embracesdk/internal/otel/spans/OtelSpanCreatorTest.kt‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import io.embrace.android.embracesdk.fakes.FakeEmbraceSdkSpan
55
import io.embrace.android.embracesdk.fakes.FakeOtelJavaTracer
66
import io.embrace.android.embracesdk.fakes.FakeOtelKotlinClock
77
import io.embrace.android.embracesdk.fakes.FakeSpanBuilder
8+
import io.embrace.android.embracesdk.fixtures.TOO_LONG_INTERNAL_SPAN_NAME
9+
import io.embrace.android.embracesdk.fixtures.TOO_LONG_SPAN_NAME
810
import io.embrace.android.embracesdk.internal.clock.millisToNanos
911
import io.embrace.android.embracesdk.internal.otel.schema.EmbType
1012
import io.embrace.android.embracesdk.internal.otel.schema.PrivateSpan
@@ -54,7 +56,7 @@ internal class OtelSpanCreatorTest {
5456
assertTrue(contains(PrivateSpan))
5557
assertTrue(contains(EmbType.Performance.Default))
5658
}
57-
assertEquals("emb-test", args.spanName)
59+
assertEquals("emb-test", args.initialSpanName)
5860
assertNull(creator.spanStartArgs.getParentSpanContext())
5961

6062
creator.startSpan(startTime).assertSpan(
@@ -168,6 +170,42 @@ internal class OtelSpanCreatorTest {
168170
assertEquals("test-value", args.customAttributes["test-key"])
169171
}
170172

173+
@Test
174+
fun `initial name not truncated`() {
175+
val tracer = TracerAdapter(tracer, otelClock)
176+
val startTime = clock.now()
177+
178+
val creator = OtelSpanCreator(
179+
tracer = tracer,
180+
spanStartArgs = OtelSpanStartArgs(
181+
name = TOO_LONG_SPAN_NAME,
182+
type = EmbType.Performance.Default,
183+
internal = false,
184+
private = false,
185+
)
186+
)
187+
188+
creator.startSpan(startTime).assertSpan(
189+
expectedName = TOO_LONG_SPAN_NAME,
190+
expectedStartTimeMs = startTime
191+
)
192+
193+
val internalSpanCreator = OtelSpanCreator(
194+
tracer = tracer,
195+
spanStartArgs = OtelSpanStartArgs(
196+
name = TOO_LONG_INTERNAL_SPAN_NAME,
197+
type = EmbType.Performance.Default,
198+
internal = true,
199+
private = false,
200+
)
201+
)
202+
203+
internalSpanCreator.startSpan(startTime).assertSpan(
204+
expectedName = "emb-$TOO_LONG_INTERNAL_SPAN_NAME",
205+
expectedStartTimeMs = startTime
206+
)
207+
}
208+
171209
private fun Span.assertSpan(
172210
expectedName: String,
173211
expectedSpanKind: SpanKind = SpanKind.INTERNAL,

0 commit comments

Comments
 (0)