Instrumentation API part 2 #408
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,12 +22,10 @@ | |
| import io.opentelemetry.android.instrumentation.common.InstrumentedApplication; | ||
| import io.opentelemetry.android.instrumentation.crash.CrashReporter; | ||
| import io.opentelemetry.android.instrumentation.crash.CrashReporterBuilder; | ||
| import io.opentelemetry.android.instrumentation.slowrendering.SlowRenderingDetector; | ||
| import io.opentelemetry.android.instrumentation.startup.InitializationEvents; | ||
| import io.opentelemetry.android.instrumentation.startup.SdkInitializationEvents; | ||
| import io.opentelemetry.android.internal.features.networkattrs.NetworkAttributesSpanAppender; | ||
| import io.opentelemetry.android.internal.features.persistence.DiskManager; | ||
| import io.opentelemetry.android.internal.features.persistence.SimpleTemporaryFileProvider; | ||
| import io.opentelemetry.android.internal.processors.GlobalAttributesLogRecordAppender; | ||
|
|
@@ -91,7 +89,6 @@ public final class OpenTelemetryRumBuilder { | |
| (a) -> a; | ||
|
|
||
| private Resource resource; | ||
| private InitializationEvents initializationEvents = InitializationEvents.NO_OP; | ||
| private Consumer<AnrDetectorBuilder> anrCustomizer = x -> {}; | ||
| private Consumer<CrashReporterBuilder> crashReporterCustomizer = x -> {}; | ||
|
|
@@ -121,18 +118,6 @@ public OpenTelemetryRumBuilder setResource(Resource resource) { | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Merges a new {@link Resource} with any existing {@link Resource} in this builder. The | ||
| * resulting {@link Resource} will be attached to all telemetry emitted by the {@link | ||
|
|
@@ -402,27 +387,17 @@ private void applyConfiguration() { | |
| // Network specific attributes | ||
| if (config.shouldIncludeNetworkAttributes()) { | ||
| // Add span processor that appends network attributes. | ||
| addTracerProviderCustomizer( | ||
| (tracerProviderBuilder, app) -> { | ||
| SpanProcessor networkAttributesSpanAppender = | ||
| NetworkAttributesSpanAppender.create( | ||
| ServiceManager.get().getCurrentNetworkProvider()); | ||
|
Comment on lines
+393
to
+394
There was a problem hiding this comment. Does this also eventually move to the network instrumentation module? It currently strange to me because the main agent is aware that some network attributes exist, and it is including them based on config, but there's no There was a problem hiding this comment. This feature seems to be only for adding global attrs with a SpanProcessor, so I think it should live in the core module to be available before initialization. I've added more details on it in this other comment. |
||
| return tracerProviderBuilder.addSpanProcessor( | ||
| networkAttributesSpanAppender); | ||
| }); | ||
| initializationEvents.currentNetworkProviderInitialized(); | ||
| } | ||
|
|
||
| // Add span processor that appends screen attribute(s) | ||
| if (config.shouldIncludeScreenAttributes()) { | ||
| addTracerProviderCustomizer( | ||
|
|
@@ -477,13 +452,6 @@ private void applyConfiguration() { | |
| } | ||
| } | ||
|
|
||
| private SdkTracerProvider buildTracerProvider( | ||
| SessionId sessionId, Application application, SpanExporter spanExporter) { | ||
| SdkTracerProviderBuilder tracerProviderBuilder = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
|
|
||
| import io.opentelemetry.android.ScreenAttributesSpanProcessor; | ||
| import io.opentelemetry.android.features.diskbuffering.DiskBufferingConfiguration; | ||
| import io.opentelemetry.android.internal.services.network.CurrentNetworkProvider; | ||
| import io.opentelemetry.api.common.Attributes; | ||
| import java.time.Duration; | ||
| import java.util.function.Supplier; | ||
|
|
@@ -28,7 +28,6 @@ public class OtelRumConfig { | |
| private boolean includeScreenAttributes = true; | ||
| private DiskBufferingConfiguration diskBufferingConfiguration = | ||
| DiskBufferingConfiguration.builder().build(); | ||
| private boolean anrDetectionEnabled = true; | ||
| private boolean slowRenderingDetectionEnabled = true; | ||
| private Duration slowRenderingDetectionPollInterval = | ||
|
|
@@ -121,20 +120,6 @@ public OtelRumConfig setDiskBufferingConfiguration( | |
| return this; | ||
| } | ||
|
|
||
|
Comment on lines
-124
to
-137
There was a problem hiding this comment. So now the configuration choice is no longer programmatic and cannot be dynamic. It is determined statically at build-time depending on whether or not the user has included the network instrumentation dependency? I think that's fine, I just wanted to think that through, out loud. If/when the need for additional configuration arises, I suppose we can handle that within each instrumentation module that requires it. There was a problem hiding this comment.
These changes mean that the core/init config is not aware of the network changes instrumentation. As it is now, it can only be enabled at build-time by adding the instrumentation dependency and it cannot be disabled if it's in the classpath at runtime. However, we could expand There was a problem hiding this comment. Cool. I think it could be helpful to have a tracking issue for this, so I've opened #411 to track this. 👍🏻 |
||
| /** Returns true if ANR (application not responding) detection is enabled (default = true). */ | ||
| public boolean isAnrDetectionEnabled() { | ||
| return anrDetectionEnabled; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.android.internal.services.network; | ||
|
|
||
| import android.os.Build; | ||
| import android.telephony.TelephonyManager; | ||
| import androidx.annotation.RequiresApi; | ||
| import io.opentelemetry.android.internal.services.network.data.Carrier; | ||
|
|
||
| /** | ||
| * This class is internal and not for public use. Its APIs are unstable and can change at any time. | ||
| */ | ||
| @RequiresApi(api = Build.VERSION_CODES.P) | ||
| public class CarrierFinder { | ||
breedx-splk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| private final TelephonyManager telephonyManager; | ||
|
|
||
| public CarrierFinder(TelephonyManager telephonyManager) { | ||
| this.telephonyManager = telephonyManager; | ||
| } | ||
|
|
||
| public Carrier get() { | ||
| int id = telephonyManager.getSimCarrierId(); | ||
| String name = null; | ||
| String mobileCountryCode = null; | ||
| String mobileNetworkCode = null; | ||
| String isoCountryCode = null; | ||
| CharSequence simCarrierIdName = telephonyManager.getSimCarrierIdName(); | ||
| if (validString(simCarrierIdName)) { | ||
| name = simCarrierIdName.toString(); | ||
| } | ||
| String simOperator = telephonyManager.getSimOperator(); | ||
| if (validString(simOperator) && simOperator.length() >= 5) { | ||
| mobileCountryCode = simOperator.substring(0, 3); | ||
| mobileNetworkCode = simOperator.substring(3); | ||
| } | ||
| String providedIsoCountryCode = telephonyManager.getSimCountryIso(); | ||
| if (validString(providedIsoCountryCode)) { | ||
| isoCountryCode = providedIsoCountryCode; | ||
| } | ||
| return new Carrier(id, name, mobileCountryCode, mobileNetworkCode, isoCountryCode); | ||
| } | ||
|
|
||
| private boolean validString(CharSequence str) { | ||
| return !(str == null || str.length() == 0); | ||
| } | ||
| } | ||
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.
It's weird to me to see this added at the same time the direct dependency on the network instrumentation goes away. Is this kinda front-loading the permission, just in case the user opts into it later? Hmmm.....if we're making the user choose which instrumentation to include, should we also require them to add the permission?
Is there a way to do this in the network instrumentation lib instead? I don't know the answer here.
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.
The network module (and also one of the lifecycle-related modules if I remember correctly) are tricky because they have 2 responsibilities. One is generating telemetry when some event occurs, and the other one is providing global attrs to be appended to all spans/logs.
The former responsibility belongs to the instrumentation API, as it's related to generating telemetry. The latter one though seems to be more related to general global attrs, which must be set during initialization when creating the OTel instance, so that part needs to live in the core of the agent. Both, the instrumentation and the global attrs responsibilities need to use the same Android's network APIs, which is why I made it a service tool (which is the one that requires this permission).
We could do that, probably we might find the RequiresPermission annotation useful for the config that enables the network global attrs feature to make users aware of it at compile time. Although we'd have to make it disabled by default then.
I think this could be possible in the future based on what I've heard about resource entities, which might help with emitting these state changes once in an event, linking them to all the rest of telemetry without having to actually send these attrs on each span/log. Other than that, another way could be to allow instrumentations to influence how the OTel instance is created, which I'm not sure it's a good idea.
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.
Ok, I think we're on the same page anyway about it being somewhat confusing that there are "network related instrumentation responsibilities" still in both the agent and the specific instrumentation. We can circle back on that another time to figure that out....but it's good that we've recognized it.
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.
Tossing in my 2 cents re: permission
It seems like the library ought not elevate the permissions the app asks for, but instead fail gracefully if permission is not granted. If for some reason someone add this erroneously to their app that should not make network calls (and thus did not ask for the permission), that will open up a hole in the app.
Not a huge deal, but it's ideal to not have to worry about that