Skip to content
Draft
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 @@ -20,6 +20,7 @@ import com.datadog.android.BuildConfig
import com.datadog.android.Datadog
import com.datadog.android.DatadogSite
import com.datadog.android.api.InternalLogger
import com.datadog.android.api.feature.Feature
import com.datadog.android.api.storage.RawBatchEvent
import com.datadog.android.core.configuration.BackPressureStrategy
import com.datadog.android.core.configuration.BatchProcessingLevel
Expand Down Expand Up @@ -234,7 +235,8 @@ internal class CoreFeature(
private val lastViewEventFileWriter: FileWriter<RawBatchEvent> by lazy {
BatchFileReaderWriter.create(
internalLogger = internalLogger,
encryption = localDataEncryption
encryption = localDataEncryption,
featureName = Feature.RUM_FEATURE_NAME
)
}

Expand Down Expand Up @@ -436,7 +438,7 @@ internal class CoreFeature(
if (lastViewEventFile == null) return null

val reader =
BatchFileReaderWriter.create(internalLogger, localDataEncryption)
BatchFileReaderWriter.create(internalLogger, localDataEncryption, Feature.RUM_FEATURE_NAME)
val content = reader.readData(lastViewEventFile)
return if (content.isEmpty()) {
null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,8 @@ internal class SdkFeature(
pendingOrchestrator = fileOrchestrator.pendingOrchestrator,
batchEventsReaderWriter = BatchFileReaderWriter.create(
internalLogger = internalLogger,
encryption = coreFeature.localDataEncryption
encryption = coreFeature.localDataEncryption,
featureName = featureName
),
batchMetadataReaderWriter = FileReaderWriter.create(
internalLogger = internalLogger,
Expand Down Expand Up @@ -454,7 +455,7 @@ internal class SdkFeature(
val flusher = DataFlusher(
contextProvider,
fileOrchestrator,
BatchFileReaderWriter.create(internalLogger, coreFeature.localDataEncryption),
BatchFileReaderWriter.create(internalLogger, coreFeature.localDataEncryption, wrappedFeature.name),
FileReaderWriter.create(internalLogger, coreFeature.localDataEncryption),
FileMover(internalLogger),
internalLogger
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ internal class ConsentAwareStorage(
return AsyncEventWriteScope(executorService, NoOpEventBatchWriter(), writeLock, featureName, internalLogger)
}
val writer = FileEventBatchWriter(
featureName = featureName,
fileOrchestrator = orchestrator,
eventsWriter = batchEventsReaderWriter,
metadataReaderWriter = batchMetadataReaderWriter,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ import com.datadog.android.core.internal.persistence.file.FilePersistenceConfig
import com.datadog.android.core.internal.persistence.file.FileReaderWriter
import com.datadog.android.core.internal.persistence.file.FileWriter
import com.datadog.android.core.internal.persistence.file.existsSafe
import com.datadog.android.internal.telemetry.TelemetryAttributes
import java.io.File
import java.util.Locale

internal class FileEventBatchWriter(
private val featureName: String,
private val fileOrchestrator: FileOrchestrator,
private val eventsWriter: FileWriter<RawBatchEvent>,
private val metadataReaderWriter: FileReaderWriter,
Expand Down Expand Up @@ -88,14 +90,18 @@ internal class FileEventBatchWriter(
if (eventSize > filePersistenceConfig.maxItemSize) {
internalLogger.log(
InternalLogger.Level.ERROR,
InternalLogger.Target.USER,
{
targets = listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),
messageBuilder = {
ERROR_LARGE_DATA.format(
Locale.US,
eventSize,
filePersistenceConfig.maxItemSize
)
}
},
additionalProperties = mapOf(
TelemetryAttributes.FEATURE_NAME to featureName,
TelemetryAttributes.EVENT_DROPPED_BYTES to eventSize
)
)
return false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ internal interface BatchFileReaderWriter : FileWriter<RawBatchEvent>, BatchFileR
* Creates either plain [PlainBatchFileReaderWriter] or [PlainBatchFileReaderWriter] wrapped in
* [EncryptedBatchReaderWriter] if encryption is provided.
*/
fun create(internalLogger: InternalLogger, encryption: Encryption?): BatchFileReaderWriter {
val readerWriter = PlainBatchFileReaderWriter(internalLogger)
fun create(
internalLogger: InternalLogger,
encryption: Encryption?,
featureName: String
): BatchFileReaderWriter {
val readerWriter = PlainBatchFileReaderWriter(internalLogger, featureName)
return if (encryption == null) {
readerWriter
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import com.datadog.android.api.InternalLogger
import com.datadog.android.api.storage.RawBatchEvent
import com.datadog.android.core.internal.persistence.file.lengthSafe
import com.datadog.android.core.internal.utils.use
import com.datadog.android.internal.telemetry.TelemetryAttributes
import java.io.File
import java.io.IOException
import java.io.InputStream
Expand All @@ -23,7 +24,8 @@ import kotlin.math.max
* Stores data in the TLV format as meta+data, use only for RUM/Log/Trace events.
*/
internal class PlainBatchFileReaderWriter(
private val internalLogger: InternalLogger
private val internalLogger: InternalLogger,
private val featureName: String
) : BatchFileReaderWriter {

// region FileWriter
Expand Down Expand Up @@ -68,18 +70,20 @@ internal class PlainBatchFileReaderWriter(
readFileData(file)
} catch (e: IOException) {
internalLogger.log(
InternalLogger.Level.ERROR,
listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY),
{ ERROR_READ.format(Locale.US, file.path) },
e
level = InternalLogger.Level.ERROR,
targets = listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY),
messageBuilder = { ERROR_READ.format(Locale.US, file.path) },
throwable = e,
additionalProperties = mapOf(TelemetryAttributes.FEATURE_NAME to featureName)
)
emptyList()
} catch (e: SecurityException) {
internalLogger.log(
InternalLogger.Level.ERROR,
listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY),
{ ERROR_READ.format(Locale.US, file.path) },
e
level = InternalLogger.Level.ERROR,
targets = listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY),
messageBuilder = { ERROR_READ.format(Locale.US, file.path) },
throwable = e,
additionalProperties = mapOf(TelemetryAttributes.FEATURE_NAME to featureName)
)
emptyList()
}
Expand Down Expand Up @@ -173,9 +177,10 @@ internal class PlainBatchFileReaderWriter(

if (remaining != 0 || (inputLength > 0 && result.isEmpty())) {
internalLogger.log(
InternalLogger.Level.ERROR,
listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),
{ WARNING_NOT_ALL_DATA_READ.format(Locale.US, file.path) }
level = InternalLogger.Level.ERROR,
targets = listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),
messageBuilder = { WARNING_NOT_ALL_DATA_READ.format(Locale.US, file.path) },
additionalProperties = mapOf(TelemetryAttributes.FEATURE_NAME to featureName)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Report dropped bytes for corrupted batch reads

When a batch file is truncated or corrupt and this branch logs WARNING_NOT_ALL_DATA_READ, the unread bytes represented by remaining are the data that will be lost once the partially read batch is confirmed/deleted. The new write-side oversized-event telemetry includes event.dropped_bytes, but this read-side drop signal only has feature.name, so read corruption remains unqueryable by loss size despite the value being available here.

Useful? React with 👍 / 👎.

)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ internal class TLVBlock(

private fun logEntrySizeExceededError(entrySize: Int, maxEntrySize: Int) {
internalLogger.log(
target = InternalLogger.Target.MAINTAINER,
level = InternalLogger.Level.WARN,
level = InternalLogger.Level.ERROR,
targets = listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),
messageBuilder = { BYTE_LENGTH_EXCEEDED_ERROR.format(Locale.US, maxEntrySize, entrySize) }
)
}

internal companion object {
// The maximum length of data (Value) in TLV block defining key data.
private const val MAXIMUM_DATA_SIZE_MB = 10 * 1024 * 1024 // 10 mb
internal const val MAXIMUM_DATA_SIZE_MB = 10 * 1024 * 1024 // 10 mb
internal const val BYTE_LENGTH_EXCEEDED_ERROR =
"DataBlock length exceeds limit of %s bytes, was %s"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import java.util.Locale

internal class TLVBlockFileReader(
val internalLogger: InternalLogger,
val fileReaderWriter: FileReaderWriter
val fileReaderWriter: FileReaderWriter,
private val maxEntrySize: Int = TLVBlock.MAXIMUM_DATA_SIZE_MB
) {
@WorkerThread
internal fun read(
Expand Down Expand Up @@ -76,6 +77,7 @@ internal class TLVBlockFileReader(
)
}

@Suppress("ReturnCount")
private fun readData(inputArray: ByteArray, currentIndex: Int): TLVResult<ByteArray>? {
val lengthBlockSize = Int.SIZE_BYTES
var newIndex = currentIndex + lengthBlockSize
Expand All @@ -89,6 +91,11 @@ internal class TLVBlockFileReader(

val lengthData = lengthInBytes.toInt()

if (lengthData !in 0..maxEntrySize) {
logBlockSizeExceededError(lengthData)
return null
}

val dataBytes =
inputArray.copyOfRangeSafe(newIndex, newIndex + lengthData)

Expand All @@ -113,6 +120,16 @@ internal class TLVBlockFileReader(
)
}

private fun logBlockSizeExceededError(lengthData: Int) {
internalLogger.log(
level = InternalLogger.Level.ERROR,
targets = listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),
messageBuilder = {
CORRUPT_DATA_LENGTH_ERROR.format(Locale.US, maxEntrySize, lengthData)
}
Comment on lines +126 to +129

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include feature name on TLV datastore telemetry

When a feature's DataStore file has an invalid declared TLV length, this new telemetry error is emitted without TelemetryAttributes.FEATURE_NAME, even though SdkFeature.prepareDataStoreHandler constructs the reader per wrappedFeature.name. As a result, corrupt datastore-read drops cannot be attributed to RUM/logs/tracing/session-replay in telemetry queries, unlike the batch read/write telemetry added in this change.

Useful? React with 👍 / 👎.

)
}

private fun logFailedToDeserializeError() {
internalLogger.log(
target = InternalLogger.Target.MAINTAINER,
Expand All @@ -129,5 +146,10 @@ internal class TLVBlockFileReader(
internal companion object {
internal const val CORRUPT_TLV_HEADER_TYPE_ERROR = "TLV header corrupt. Invalid type %s"
internal const val FAILED_TO_DESERIALIZE_ERROR = "Failed to deserialize TLV data length"

// Read-side counterpart of TLVBlock.BYTE_LENGTH_EXCEEDED_ERROR (write/serialize side).
// Kept distinct so dropped blocks can be told apart by direction in telemetry.
internal const val CORRUPT_DATA_LENGTH_ERROR =
"DataBlock length read from file is invalid or exceeds limit of %s bytes, was %s"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import android.net.ConnectivityManager
import android.os.Process
import com.datadog.android.Datadog
import com.datadog.android.api.InternalLogger
import com.datadog.android.api.feature.Feature
import com.datadog.android.api.storage.RawBatchEvent
import com.datadog.android.core.configuration.Configuration
import com.datadog.android.core.internal.net.info.BroadcastReceiverNetworkInfoProvider
Expand Down Expand Up @@ -1285,7 +1286,7 @@ internal class CoreFeatureTest {
legacyNdkViewEventFile.parentFile?.mkdirs()

BatchFileReaderWriter
.create(internalLogger = mock(), encryption = null)
.create(internalLogger = mock(), encryption = null, featureName = Feature.RUM_FEATURE_NAME)
.writeData(
legacyNdkViewEventFile,
RawBatchEvent(fakeViewEvent.toString().toByteArray()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import com.datadog.android.core.internal.persistence.file.FileOrchestrator
import com.datadog.android.core.internal.persistence.file.FilePersistenceConfig
import com.datadog.android.core.internal.persistence.file.FileReaderWriter
import com.datadog.android.core.internal.persistence.file.FileWriter
import com.datadog.android.internal.telemetry.TelemetryAttributes
import com.datadog.android.utils.forge.Configurator
import com.datadog.android.utils.verifyLog
import fr.xgouchet.elmyr.Forge
Expand Down Expand Up @@ -80,9 +81,13 @@ internal class FileEventBatchWriterTest {
@Forgery
lateinit var fakeEventType: EventType

@StringForgery
lateinit var fakeFeatureName: String

@BeforeEach
fun `set up`() {
testedWriter = FileEventBatchWriter(
featureName = fakeFeatureName,
fileOrchestrator = mockFileOrchestrator,
eventsWriter = mockBatchWriter,
metadataReaderWriter = mockMetaReaderWriter,
Expand Down Expand Up @@ -192,11 +197,15 @@ internal class FileEventBatchWriterTest {

mockInternalLogger.verifyLog(
InternalLogger.Level.ERROR,
InternalLogger.Target.USER,
listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),
ERROR_LARGE_DATA.format(
Locale.US,
batchEvent.data.size,
maxItemSize
),
additionalProperties = mapOf(
TelemetryAttributes.FEATURE_NAME to fakeFeatureName,
TelemetryAttributes.EVENT_DROPPED_BYTES to batchEvent.data.size
)
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ package com.datadog.android.core.internal.persistence.file.batch
import com.datadog.android.api.InternalLogger
import com.datadog.android.security.Encryption
import com.datadog.android.utils.forge.Configurator
import fr.xgouchet.elmyr.annotation.StringForgery
import fr.xgouchet.elmyr.junit5.ForgeConfiguration
import fr.xgouchet.elmyr.junit5.ForgeExtension
import org.assertj.core.api.Assertions.assertThat
Expand All @@ -32,10 +33,13 @@ internal class BatchFileReaderWriterTest {
@Mock
lateinit var mockInternalLogger: InternalLogger

@StringForgery
lateinit var fakeFeatureName: String

@Test
fun `M create BatchFileReaderWriter W create() { without encryption }`() {
// When
val readerWriter = BatchFileReaderWriter.create(mockInternalLogger, null)
val readerWriter = BatchFileReaderWriter.create(mockInternalLogger, null, fakeFeatureName)
// Then
assertThat(readerWriter)
.isInstanceOf(PlainBatchFileReaderWriter::class.java)
Expand All @@ -47,7 +51,8 @@ internal class BatchFileReaderWriterTest {
val mockEncryption = mock<Encryption>()
val readerWriter = BatchFileReaderWriter.create(
mockInternalLogger,
mockEncryption
mockEncryption,
fakeFeatureName
)

// Then
Expand Down
Loading
Loading