-
Notifications
You must be signed in to change notification settings - Fork 64
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
RUM-6033 Add telemetry and logs related with RumMonitor#addViewLoadigTime API #2267
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import com.datadog.android.api.storage.DataWriter | |
import com.datadog.android.core.InternalSdkCore | ||
import com.datadog.android.core.internal.net.FirstPartyHostHeaderTypeResolver | ||
import com.datadog.android.core.metrics.MethodCallSamplingRate | ||
import com.datadog.android.internal.telemetry.InternalTelemetryEvent | ||
import com.datadog.android.rum.DdRumContentProvider | ||
import com.datadog.android.rum.internal.anr.ANRException | ||
import com.datadog.android.rum.internal.domain.RumContext | ||
|
@@ -144,7 +145,24 @@ internal class RumViewManagerScope( | |
val importanceForeground = ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREGROUND | ||
val isForegroundProcess = processFlag == importanceForeground | ||
|
||
if (applicationDisplayed || !isForegroundProcess) { | ||
if (event is RumRawEvent.AddViewLoadingTime) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /question do you need to add checks like this in future if you any other usage telemetry like if yes, then we should change it to more generic else these if-else will explode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not anticipating that but this whole part will need re - factory soon but not planned for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a backlog item would be fine here so we don't forget. |
||
val internalLogger = sdkCore.internalLogger | ||
internalLogger.log( | ||
InternalLogger.Level.WARN, | ||
InternalLogger.Target.USER, | ||
{ MESSAGE_MISSING_VIEW } | ||
) | ||
internalLogger.logApiUsage( | ||
InternalTelemetryEvent.ApiUsage.AddViewLoadingTime( | ||
overwrite = event.overwrite, | ||
noView = true, | ||
noActiveView = false | ||
) | ||
) | ||
// we should return here and not add the event to the session ended metric missed events as we already | ||
// send the API usage telemetry | ||
return | ||
} else if (applicationDisplayed || !isForegroundProcess) { | ||
handleBackgroundEvent(event, writer) | ||
} else { | ||
val isSilentOrphanEvent = event.javaClass in silentOrphanEventTypes | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import com.datadog.android.rum.utils.forge.Configurator | |
import com.datadog.android.telemetry.model.TelemetryConfigurationEvent | ||
import com.datadog.android.telemetry.model.TelemetryDebugEvent | ||
import com.datadog.android.telemetry.model.TelemetryErrorEvent | ||
import com.datadog.android.telemetry.model.TelemetryUsageEvent | ||
import com.datadog.tools.unit.assertj.JsonObjectAssert.Companion.assertThat | ||
import com.datadog.tools.unit.forge.anException | ||
import com.google.gson.JsonObject | ||
|
@@ -33,6 +34,7 @@ import org.junit.jupiter.api.RepeatedTest | |
import org.junit.jupiter.api.Test | ||
import org.junit.jupiter.api.extension.ExtendWith | ||
import org.junit.jupiter.api.extension.Extensions | ||
import org.junit.jupiter.api.fail | ||
import org.mockito.Mock | ||
import org.mockito.junit.jupiter.MockitoExtension | ||
import org.mockito.junit.jupiter.MockitoSettings | ||
|
@@ -748,6 +750,110 @@ internal class RumEventSerializerTest { | |
} | ||
} | ||
|
||
@RepeatedTest(8) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in this case we need a repeated test as this model has multiple variables variation. Is good to have repetitive tests in order to make sure we cover more cases in this variation. |
||
fun `M serialize RUM event W serialize() with TelemetryUsageEvent`( | ||
@Forgery event: TelemetryUsageEvent | ||
) { | ||
val serialized = testedSerializer.serialize(event) | ||
val jsonObject = JsonParser.parseString(serialized).asJsonObject | ||
assertThat(jsonObject) | ||
.hasField("type", "telemetry") | ||
.hasField("_dd") { | ||
hasField("format_version", 2L) | ||
} | ||
.hasField("date", event.date) | ||
.hasField("source", event.source.name.lowercase(Locale.US).replace('_', '-')) | ||
.hasField("service", event.service) | ||
.hasField("version", event.version) | ||
.hasField("telemetry") { | ||
hasField("usage") { | ||
when (event.telemetry.usage) { | ||
is TelemetryUsageEvent.Usage.AddViewLoadingTime -> { | ||
val usage = event.telemetry.usage as TelemetryUsageEvent.Usage.AddViewLoadingTime | ||
hasField("no_view", usage.noView) | ||
hasField("no_active_view", usage.noActiveView) | ||
hasField("overwritten", usage.overwritten) | ||
} | ||
|
||
else -> { | ||
fail("Usage type not covered in assertions") | ||
} | ||
} | ||
} | ||
if (event.telemetry.device != null) { | ||
hasField("device") { | ||
val device = event.telemetry.device | ||
checkNotNull(device) | ||
if (device.architecture != null) { | ||
hasField("architecture", device.architecture!!) | ||
} | ||
if (device.brand != null) { | ||
hasField("brand", device.brand!!) | ||
} | ||
if (device.model != null) { | ||
hasField("model", device.model!!) | ||
} | ||
} | ||
} | ||
if (event.telemetry.os != null) { | ||
hasField("os") { | ||
val os = event.telemetry.os | ||
checkNotNull(os) | ||
if (os.build != null) { | ||
hasField("build", os.build!!) | ||
} | ||
if (os.name != null) { | ||
hasField("name", os.name!!) | ||
} | ||
if (os.version != null) { | ||
hasField("version", os.version!!) | ||
} | ||
} | ||
} | ||
containsAttributes(event.telemetry.additionalProperties) | ||
} | ||
|
||
val application = event.application | ||
if (application != null) { | ||
assertThat(jsonObject) | ||
.hasField("application") { | ||
hasField("id", application.id) | ||
} | ||
} else { | ||
assertThat(jsonObject).doesNotHaveField("application") | ||
} | ||
|
||
val session = event.session | ||
if (session != null) { | ||
assertThat(jsonObject) | ||
.hasField("session") { | ||
hasField("id", session.id) | ||
} | ||
} else { | ||
assertThat(jsonObject).doesNotHaveField("session") | ||
} | ||
|
||
val view = event.view | ||
if (view != null) { | ||
assertThat(jsonObject) | ||
.hasField("view") { | ||
hasField("id", view.id) | ||
} | ||
} else { | ||
assertThat(jsonObject).doesNotHaveField("view") | ||
} | ||
|
||
val action = event.action | ||
if (action != null) { | ||
assertThat(jsonObject) | ||
.hasField("action") { | ||
hasField("id", action.id) | ||
} | ||
} else { | ||
assertThat(jsonObject).doesNotHaveField("action") | ||
} | ||
} | ||
|
||
@Test | ||
fun `M serialize RUM event W serialize() with unknown event`( | ||
@Forgery unknownEvent: UserInfo | ||
|
@@ -1451,25 +1557,29 @@ internal class RumEventSerializerTest { | |
usr = (it.usr ?: ViewEvent.Usr()).copy(additionalProperties = userAttributes) | ||
) | ||
} | ||
|
||
2 -> this.getForgery(ActionEvent::class.java).let { | ||
it.copy( | ||
context = ActionEvent.Context(additionalProperties = attributes), | ||
usr = (it.usr ?: ActionEvent.Usr()).copy(additionalProperties = userAttributes) | ||
) | ||
} | ||
|
||
3 -> this.getForgery(ErrorEvent::class.java).let { | ||
it.copy( | ||
context = ErrorEvent.Context(additionalProperties = attributes), | ||
usr = (it.usr ?: ErrorEvent.Usr()).copy(additionalProperties = userAttributes) | ||
) | ||
} | ||
|
||
4 -> this.getForgery(ResourceEvent::class.java).let { | ||
it.copy( | ||
context = ResourceEvent.Context(additionalProperties = attributes), | ||
usr = (it.usr ?: ResourceEvent.Usr()) | ||
.copy(additionalProperties = userAttributes) | ||
) | ||
} | ||
|
||
else -> this.getForgery(LongTaskEvent::class.java).let { | ||
it.copy( | ||
context = LongTaskEvent.Context(additionalProperties = attributes), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Nitpick, this should be a constant and not a hardcoded Magic Number