Skip to content

Commit 10e070c

Browse files
0xnm0xnm
authored
Merge pull request #3052 from DataDog/nogorodnikov/rum-12069/fix-service-in-logs-ddtags
RUM-12069: Fix service handling in `ddtags` of `LogEvent`
2 parents ed7f4e1 + 21527b5 commit 10e070c

File tree

10 files changed

+200
-24
lines changed

10 files changed

+200
-24
lines changed

‎dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/constraints/DatadogDataConstraints.kt‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -214,8 +214,7 @@ class DatadogDataConstraints(private val internalLogger: InternalLogger) : DataC
214214
private val reservedTagKeys = setOf(
215215
"host",
216216
"device",
217-
"source",
218-
"service"
217+
"source"
219218
)
220219
}
221220
}

‎dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/constraints/DatadogDataConstraintsTest.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ internal class DatadogDataConstraintsTest {
155155

156156
@Test
157157
fun `ignore reserved tag keys`(forge: Forge) {
158-
val key = forge.anElementFrom("host", "device", "source", "service")
158+
val key = forge.anElementFrom("host", "device", "source")
159159
val value = forge.aNumericalString()
160160
val invalidTag = "$key:$value"
161161

@@ -172,7 +172,7 @@ internal class DatadogDataConstraintsTest {
172172

173173
@Test
174174
fun `ignore reserved tag keys (workaround)`(forge: Forge) {
175-
val key = forge.randomizeCase { anElementFrom("host", "device", "source", "service") }
175+
val key = forge.randomizeCase { anElementFrom("host", "device", "source") }
176176
val value = forge.aNumericalString()
177177
val invalidTag = "$key:$value"
178178

‎detekt_custom_safe_calls.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,7 @@ datadog:
696696
- "kotlin.collections.List.toMap()"
697697
- "kotlin.collections.List.toMutableList()"
698698
- "kotlin.collections.List.toMutableMap()"
699+
- "kotlin.collections.List.toMutableSet()"
699700
- "kotlin.collections.List.toSet()"
700701
- "kotlin.collections.List.toTypedArray()"
701702
- "kotlin.collections.List.withIndex()"
@@ -854,6 +855,7 @@ datadog:
854855
- "kotlin.collections.Set.any(kotlin.Function1)"
855856
- "kotlin.collections.Set.associate(kotlin.Function1)"
856857
- "kotlin.collections.Set.contains(com.datadog.android.trace.TracingHeaderType)"
858+
- "kotlin.collections.Set.filter(kotlin.Function1)"
857859
- "kotlin.collections.Set.firstOrNull(kotlin.Function1)"
858860
- "kotlin.collections.Set.forEach(kotlin.Function1)"
859861
- "kotlin.collections.Set.ifEmpty(kotlin.Function0)"

‎features/dd-sdk-android-logs/src/main/kotlin/com/datadog/android/log/Logger.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,8 @@ internal constructor(internal var handler: LogHandler) {
345345
sdkCore = sdkCore,
346346
loggerName = loggerName ?: logsFeature.packageName,
347347
logGenerator = DatadogLogGenerator(
348-
serviceName ?: sdkCore.service
348+
serviceName ?: sdkCore.service,
349+
sdkCore.internalLogger
349350
),
350351
writer = logsFeature.dataWriter,
351352
minLogPriority = minDatadogLogsPriority,

‎features/dd-sdk-android-logs/src/main/kotlin/com/datadog/android/log/internal/LogsFeature.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ internal class LogsFeature(
4444
internal var dataWriter: DataWriter<LogEvent> = NoOpDataWriter()
4545
private val initialized = AtomicBoolean(false)
4646
internal var packageName = ""
47-
private val logGenerator = DatadogLogGenerator()
47+
private val logGenerator = DatadogLogGenerator(internalLogger = sdkCore.internalLogger)
4848
private val attributes = ConcurrentHashMap<String, Any?>()
4949

5050
// region Context Information (attributes)

‎features/dd-sdk-android-logs/src/main/kotlin/com/datadog/android/log/internal/domain/DatadogLogGenerator.kt‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
package com.datadog.android.log.internal.domain
88

9+
import com.datadog.android.api.InternalLogger
910
import com.datadog.android.api.context.AccountInfo
1011
import com.datadog.android.api.context.DatadogContext
1112
import com.datadog.android.api.context.DeviceInfo
@@ -18,13 +19,15 @@ import com.datadog.android.log.LogAttributes
1819
import com.datadog.android.log.internal.utils.buildLogDateFormat
1920
import com.datadog.android.log.model.LogEvent
2021
import java.util.Date
22+
import java.util.Locale
2123

2224
@Suppress("TooManyFunctions")
2325
internal class DatadogLogGenerator(
2426
/**
2527
* Custom service name. If not provided, value will be taken from [DatadogContext].
2628
*/
27-
internal val serviceName: String? = null
29+
internal val serviceName: String? = null,
30+
private val internalLogger: InternalLogger
2831
) : LogGenerator {
2932

3033
private val simpleDateFormat = buildLogDateFormat()
@@ -262,7 +265,7 @@ internal class DatadogLogGenerator(
262265
}
263266

264267
private fun serviceTag(datadogContext: DatadogContext): String? {
265-
val service = datadogContext.service
268+
val service = serviceName ?: datadogContext.service
266269
return if (service.isNotEmpty()) {
267270
"${LogAttributes.SERVICE}:$service"
268271
} else {
@@ -316,7 +319,24 @@ internal class DatadogLogGenerator(
316319
datadogContext: DatadogContext,
317320
tags: Set<String>
318321
): MutableSet<String> {
319-
val combinedTags = mutableSetOf<String>().apply { addAll(tags) }
322+
val combinedTags = tags
323+
.filter {
324+
val key = it.split(":").firstOrNull()
325+
if (key != null && key in RESERVED_TAG_KEYS) {
326+
internalLogger.log(
327+
InternalLogger.Level.WARN,
328+
InternalLogger.Target.USER,
329+
{ RESERVED_TAG_DROPPED_WARNING.format(Locale.US, it) },
330+
onlyOnce = true
331+
)
332+
false
333+
} else {
334+
true
335+
}
336+
}
337+
.toMutableSet()
338+
339+
// when doing changes below check also DatadogDataConstraints.reservedTagKeys
320340
envTag(datadogContext)?.let {
321341
combinedTags.add(it)
322342
}
@@ -391,5 +411,13 @@ internal class DatadogLogGenerator(
391411
companion object {
392412
internal const val ISO_8601 = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"
393413
internal const val CRASH: Int = 9
414+
internal const val RESERVED_TAG_DROPPED_WARNING = "User-provided tag %s is dropped," +
415+
" because it matches reserved tag key"
416+
internal val RESERVED_TAG_KEYS = setOf(
417+
LogAttributes.ENV,
418+
LogAttributes.APPLICATION_VERSION,
419+
LogAttributes.VARIANT,
420+
LogAttributes.SERVICE
421+
)
394422
}
395423
}

‎features/dd-sdk-android-logs/src/test/kotlin/com/datadog/android/log/internal/domain/DatadogLogGeneratorTest.kt‎

Lines changed: 103 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
package com.datadog.android.log.internal.domain
88

9+
import com.datadog.android.api.InternalLogger
910
import com.datadog.android.api.context.AccountInfo
1011
import com.datadog.android.api.context.DatadogContext
1112
import com.datadog.android.api.context.NetworkInfo
@@ -30,9 +31,16 @@ import org.junit.jupiter.api.BeforeEach
3031
import org.junit.jupiter.api.Test
3132
import org.junit.jupiter.api.extension.ExtendWith
3233
import org.junit.jupiter.api.extension.Extensions
34+
import org.mockito.Mock
3335
import org.mockito.junit.jupiter.MockitoExtension
3436
import org.mockito.junit.jupiter.MockitoSettings
37+
import org.mockito.kotlin.argumentCaptor
38+
import org.mockito.kotlin.eq
39+
import org.mockito.kotlin.isNull
40+
import org.mockito.kotlin.times
41+
import org.mockito.kotlin.verify
3542
import org.mockito.quality.Strictness
43+
import java.util.Locale
3644
import java.util.UUID
3745

3846
@Extensions(
@@ -45,6 +53,9 @@ internal class DatadogLogGeneratorTest {
4553

4654
lateinit var testedLogGenerator: DatadogLogGenerator
4755

56+
@Mock
57+
lateinit var mockInternalLogger: InternalLogger
58+
4859
lateinit var fakeServiceName: String
4960
lateinit var fakeLoggerName: String
5061
lateinit var fakeAttributes: Map<String, Any?>
@@ -121,7 +132,8 @@ internal class DatadogLogGeneratorTest {
121132
)
122133

123134
testedLogGenerator = DatadogLogGenerator(
124-
fakeServiceName
135+
fakeServiceName,
136+
mockInternalLogger
125137
)
126138
}
127139

@@ -188,7 +200,7 @@ internal class DatadogLogGeneratorTest {
188200
@Test
189201
fun `M add the service name from Datadog context W creating the Log { no service name }`() {
190202
// WHEN
191-
testedLogGenerator = DatadogLogGenerator()
203+
testedLogGenerator = DatadogLogGenerator(internalLogger = mockInternalLogger)
192204
val log = testedLogGenerator.generateLog(
193205
fakeLevel,
194206
fakeLogMessage,
@@ -691,7 +703,8 @@ internal class DatadogLogGeneratorTest {
691703
fun `M not add the networkInfo W creating Log {networkInfoProvider is null}`() {
692704
// GIVEN
693705
testedLogGenerator = DatadogLogGenerator(
694-
fakeServiceName
706+
fakeServiceName,
707+
mockInternalLogger
695708
)
696709
// WHEN
697710
val log = testedLogGenerator.generateLog(
@@ -717,7 +730,8 @@ internal class DatadogLogGeneratorTest {
717730
) {
718731
// GIVEN
719732
testedLogGenerator = DatadogLogGenerator(
720-
fakeServiceName
733+
fakeServiceName,
734+
mockInternalLogger
721735
)
722736
// WHEN
723737
val log = testedLogGenerator.generateLog(
@@ -738,6 +752,60 @@ internal class DatadogLogGeneratorTest {
738752
assertThat(log).hasNetworkInfo(fakeCustomNetworkInfo)
739753
}
740754

755+
// region tags
756+
757+
@Test
758+
fun `M log warning and drop W user tag matches reserved`(
759+
forge: Forge
760+
) {
761+
// Given
762+
val fakeUserReservedTags = DatadogLogGenerator.RESERVED_TAG_KEYS
763+
.map { "$it:${forge.anAlphabeticalString()}" }
764+
765+
// When
766+
val log = testedLogGenerator.generateLog(
767+
fakeLevel,
768+
fakeLogMessage,
769+
fakeThrowable,
770+
fakeAttributes,
771+
fakeTags + fakeUserReservedTags,
772+
fakeTimestamp,
773+
fakeThreadName,
774+
fakeDatadogContext,
775+
attachNetworkInfo = true,
776+
fakeLoggerName
777+
)
778+
779+
// Then
780+
val deserializedTags = log.ddtags.split(",")
781+
assertThat(deserializedTags)
782+
.contains("${LogAttributes.APPLICATION_VERSION}:${fakeDatadogContext.version}")
783+
.contains("${LogAttributes.VARIANT}:${fakeDatadogContext.variant}")
784+
.contains("${LogAttributes.ENV}:${fakeDatadogContext.env}")
785+
.contains("${LogAttributes.SERVICE}:$fakeServiceName")
786+
787+
argumentCaptor<() -> String> {
788+
verify(mockInternalLogger, times(DatadogLogGenerator.RESERVED_TAG_KEYS.size))
789+
.log(
790+
level = eq(InternalLogger.Level.WARN),
791+
target = eq(InternalLogger.Target.USER),
792+
messageBuilder = capture(),
793+
throwable = isNull(),
794+
onlyOnce = eq(true),
795+
additionalProperties = isNull()
796+
)
797+
798+
val messages = allValues.map { it() }
799+
val expectedMessages = fakeUserReservedTags.map {
800+
DatadogLogGenerator.RESERVED_TAG_DROPPED_WARNING.format(
801+
Locale.US,
802+
it
803+
)
804+
}
805+
assertThat(messages).isEqualTo(expectedMessages)
806+
}
807+
}
808+
741809
@Test
742810
fun `M add the envNameTag W not empty`() {
743811
// WHEN
@@ -785,7 +853,7 @@ internal class DatadogLogGeneratorTest {
785853
val expectedTags = fakeTags +
786854
"${LogAttributes.APPLICATION_VERSION}:${fakeDatadogContext.version}" +
787855
"${LogAttributes.VARIANT}:${fakeDatadogContext.variant}" +
788-
"${LogAttributes.SERVICE}:${fakeDatadogContext.service}"
856+
"${LogAttributes.SERVICE}:$fakeServiceName"
789857
assertThat(log).hasExactlyTags(expectedTags)
790858
}
791859

@@ -836,7 +904,7 @@ internal class DatadogLogGeneratorTest {
836904
val expectedTags = fakeTags +
837905
"${LogAttributes.ENV}:${fakeDatadogContext.env}" +
838906
"${LogAttributes.VARIANT}:${fakeDatadogContext.variant}" +
839-
"${LogAttributes.SERVICE}:${fakeDatadogContext.service}"
907+
"${LogAttributes.SERVICE}:$fakeServiceName"
840908
assertThat(log).hasExactlyTags(expectedTags)
841909
}
842910

@@ -887,7 +955,7 @@ internal class DatadogLogGeneratorTest {
887955
val expectedTags = fakeTags +
888956
"${LogAttributes.ENV}:${fakeDatadogContext.env}" +
889957
"${LogAttributes.APPLICATION_VERSION}:${fakeDatadogContext.version}" +
890-
"${LogAttributes.SERVICE}:${fakeDatadogContext.service}"
958+
"${LogAttributes.SERVICE}:$fakeServiceName"
891959
assertThat(log).hasExactlyTags(expectedTags)
892960
}
893961

@@ -907,6 +975,31 @@ internal class DatadogLogGeneratorTest {
907975
fakeLoggerName
908976
)
909977

978+
// Then
979+
val deserializedTags = log.ddtags.split(",")
980+
assertThat(deserializedTags)
981+
.contains("${LogAttributes.SERVICE}:$fakeServiceName")
982+
}
983+
984+
@Test
985+
fun `M add the serviceTag W not empty { custom service name not provided }`() {
986+
// Given
987+
testedLogGenerator = DatadogLogGenerator(internalLogger = mockInternalLogger)
988+
989+
// When
990+
val log = testedLogGenerator.generateLog(
991+
fakeLevel,
992+
fakeLogMessage,
993+
fakeThrowable,
994+
fakeAttributes,
995+
fakeTags,
996+
fakeTimestamp,
997+
fakeThreadName,
998+
fakeDatadogContext,
999+
attachNetworkInfo = true,
1000+
fakeLoggerName
1001+
)
1002+
9101003
// Then
9111004
val deserializedTags = log.ddtags.split(",")
9121005
assertThat(deserializedTags)
@@ -916,6 +1009,7 @@ internal class DatadogLogGeneratorTest {
9161009
@Test
9171010
fun `M not add the serviceTag W empty`() {
9181011
// Given
1012+
testedLogGenerator = DatadogLogGenerator(internalLogger = mockInternalLogger)
9191013
fakeDatadogContext = fakeDatadogContext.copy(
9201014
service = ""
9211015
)
@@ -962,6 +1056,8 @@ internal class DatadogLogGeneratorTest {
9621056
assertThat(log).hasDeviceArchitecture(fakeDatadogContext.deviceInfo.architecture)
9631057
}
9641058

1059+
// endregion
1060+
9651061
@Test
9661062
fun `M bundle the trace information W required`() {
9671063
// WHEN

‎features/dd-sdk-android-logs/src/test/kotlin/com/datadog/android/log/internal/domain/event/LogEventSerializerTest.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ internal class LogEventSerializerTest {
7373
fun `ignores reserved tags keys`(@Forgery fakeLog: LogEvent, forge: Forge) {
7474
// Given
7575
val logWithoutTags = fakeLog.copy(ddtags = "")
76-
val key = forge.anElementFrom("host", "device", "source", "service")
76+
val key = forge.anElementFrom("host", "device", "source")
7777
val value = forge.aNumericalString()
7878
val reservedTag = "$key:$value"
7979
val logWithReservedTags = fakeLog.copy(ddtags = reservedTag)

0 commit comments

Comments
 (0)