Instrumentation API part 2 #408

LikeTheSalad · 2024-06-03T15:44:41Z

These changes involve the telemetry sent around network status changes. The amount of files is large mostly because of several files, prod code and unit tests, moved over to android-agent (to become part of a service).

Since the relationship around the instrumentations and the android-agent module is going to be inverted and a couple of tools needed across multiple modules will be turned into services, I realized that it's going to be very difficult to keep the build passing without having all the instrumentation modules already migrated over to the new API. Also, as soon as the first instrumentation is migrated (like this one), the android-agent will no longer provide that functionality out of the box, as now the dependency relationship is inverted, which breaks the current behavior, so in order to avoid causing troubles while all the instrumentation modules are migrated, I've created a feature branch to merge all the PRs into, one instrumentation at a time, so that they are easier to review and won't break the current functionality.

breedx-splk

Looks good to me. I do want to figure out the dependency direction issue tho.

...ork/src/main/java/io/opentelemetry/android/instrumentation/network/NetworkChangeMonitor.java

breedx-splk · 2024-06-03T17:33:46Z

android-agent/src/main/AndroidManifest.xml

@@ -1,6 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <manifest xmlns:android="http://schemas.android.com/apk/res/android">

+    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />


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.

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).

if we're making the user choose which instrumentation to include, should we also require them to add the 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.

Is there a way to do this in the network instrumentation lib instead?

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.

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.

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

breedx-splk · 2024-06-03T17:38:56Z

instrumentation/network/build.gradle.kts

@@ -21,7 +21,7 @@ android {
 dependencies {
    api(platform(libs.opentelemetry.platform))
    api(libs.opentelemetry.api)
-    api(project(":common"))
+    api(project(":android-agent"))


This contradicts my understanding of the original design:

It's odd to me that the specific instrumentation now depends on the topmost agent. Is this something that goes away in part 3 or something?

The general idea is to avoid the core parts of the agent having a dependency on the instrumentations. I agree it's confusing as it's shown here. My plan after all the instrumentations are created is to rename the existing android-agent module to something else, not sure yet, probably core, and then create a new android-agent module which will include the core along with all the instrumentations that we'd like the agent to have "enabled" out of the box.

In the future, it would be nice for the Agent to not have an explicit dependency on the instrumentation modules. But that will require an interface layer using which instrumentations can be enabled/disabled/configured via the Agent.

Something something Gradle plugin for build time module inclusion/exclusion based on a config file :-)

In the future, it would be nice for the Agent to not have an explicit dependency on the instrumentation modules. But that will require an interface layer using which instrumentations can be enabled/disabled/configured via the Agent.

It's my understanding that this is literally what @LikeTheSalad is working on here -- by making the instrumentations implement the API, they can be service loaded via classpath at runtime, and the install API is lightweight and dependency free (at least from the agent to the instrumentation). It's happening.

Yes, as @breedx-splk mentioned, avoiding the core part of the agent depending on the instrumentation modules is being done in these PRs. Though it's done in a way that, in the end, end users who add the android-agent dependency into their projects will still get all the current default instrumentations enabled because the final android-agent module will have a dependency on the instrumentations, alongside the "core" module. I did it this way to keep the same behavior somewhat unchanged after the refactoring (at least when it comes to adding the agent dependency into a project). What I understood from what @bidetofevil was mentioning is to use a different approach to include the default instrumentations in the future if I understood correctly? Maybe via a grade plugin that adds the instrumentation dependencies directly into the host project or the like. I haven't thought much about alternatives to add the default instrumentations tbh (these PRs are consuming most of my energy for now) though if we need further work that could keep on improving it I'm up to discuss it once I'm done with these PRs 😅 also because I think it's better not to add too many breaking changes at once.

I think what I imagined the eventual state to be is what the diagram outlines - there being no direct or indirect dependency from the eventual Agent module on any module that contains instrumentation implementations. If that's what the eventual state will be, that's great - apologies for missing the context.

The second line is a completely different thought, and like @LikeTheSalad described, is more about the App using the Agent not having to explicitly include the modules in its gradle files, but rather have it automatically pulled in with a gradle plugin. This will allow us to have a central location to manage versioning instead of making the app update. But this is very much a forward-looking dream and not something that should be included in this PR or even in any short term plans 😅

I think what I imagined the eventual state to be is what the diagram outlines - there being no direct or indirect dependency from the eventual Agent module on any module that contains instrumentation implementations.

This is what will happen with the existing android-agent module, which at some point will be renamed to something like core or similar to denote the core/init functionality of the agent. Now, the new android-agent module will have a dependency on core plus all the instrumentation modules that we want to provide by default. This is because of what we discussed some time ago about providing some instrumentations by default that we thought people would expect the agent to have out of the box. The only way I think we could avoid any dependencies on the agent, while still being able to provide default instrumentations, would be via a gradle plugin that adds those dependencies straight into the host project, though I agree that it sounds like something for a not-so-near future as I think we should discuss the pros/cons/alternatives in detail first.

For now, I'm trying to keep overall functionality somewhat intact, hence the module renaming to core to still provide a library named android-agent with the same default instrumentations as the current android-agent provides.

Understood. Maybe we can discuss this further in the future - I believe we can make it so that eventually, the whole of the Agent co de do not need to know about or directly depending on any specific instrumentation's implementation code. But that won't be for right now and may not be worth it, so we can move on 😅

breedx-splk · 2024-06-03T17:47:20Z

android-agent/src/main/java/io/opentelemetry/android/OpenTelemetryRumBuilder.java

+                                NetworkAttributesSpanAppender.create(
+                                        ServiceManager.get().getCurrentNetworkProvider());


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
clear indication about where these network attributes originate (hint: it's in a separate instrumentation module).

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.

breedx-splk · 2024-06-03T17:49:43Z

android-agent/src/main/java/io/opentelemetry/android/config/OtelRumConfig.java

-    /**
-     * Sets the configuration so that network change monitoring, which is enabled by default, will
-     * not be started.
-     */
-    public OtelRumConfig disableNetworkChangeMonitoring() {
-        networkChangeMonitoringEnabled = false;
-        return this;
-    }
-
-    /** Returns true if network change monitoring is enabled (default = true). */
-    public boolean isNetworkChangeMonitoringEnabled() {
-        return networkChangeMonitoringEnabled;
-    }
-


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.

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?

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 AndroidInstrumentation to allow for disabling an instrumentation, even if it was already enabled (by using the suppress context key) - Or we could also avoid loading instrumentations altogether from the classpath, and instead require each instrumentation to be manually "enabled" by registering it through the AndroidInstrumentaionRegistry. So yeah it's as you mentioned, I think we can handle different use cases if the need arises.

Cool. I think it could be helpful to have a tracking issue for this, so I've opened #411 to track this. 👍🏻

...id-agent/src/main/java/io/opentelemetry/android/internal/services/network/CarrierFinder.java

...src/main/java/io/opentelemetry/android/internal/services/network/CurrentNetworkProvider.java

breedx-splk · 2024-06-03T17:58:57Z

...oid-agent/src/main/java/io/opentelemetry/android/internal/services/network/data/Carrier.java

@@ -12,15 +12,15 @@
 import java.util.Objects;

 @RequiresApi(api = Build.VERSION_CODES.P)
-final class Carrier {
+public final class Carrier {


Add disclaimer about internal package.

Might be a nice choice for a kotlin data class? Certainly can wait tho.

Thanks, I've made the changes.

breedx-splk · 2024-06-04T18:06:08Z

android-agent/src/main/java/io/opentelemetry/android/internal/services/network/data/Carrier.kt

+        isoCountryCode = builder.isoCountryCode,
+    )
+
+    class Builder {


I think we want to not use builders for data classes. :) I think we should just give the constructor args default values and remove the builder.

Thanks, I've made the changes. Although (for future reference) I think it's worth mentioning that the idea of "default args" == "builder" is not always true to me. It seems to come from the general perception that the builder pattern is a java-specific one and so it becomes unnecessary elsewhere, however, when building immutable objects for which you don't know what args are available at compile-time, or for when needed to create an instance that needs to be "visited/mutated" in different places before being ready, I believe the builder pattern makes sense in any object oriented language that supports immutability.

I think this specific use case is a good example, it's only used in a single place (CarrierFinder), and the available args are not known at compile-time, so the benefit that the default args provide is not used, as we still need to pass all of them because we don't know which ones will be available, and to verify which ones are there, we have to create variables for each arg and set them whenever possible (essentially doing the same as the builder does but on the caller side, which is not practical if needed to be done in different places, fortunately it was only needed once) as shown here.

This is probably a matter of personal taste, but I find long-living builders that get passed around for various piece of code to contribute to the init data to be confusing and potentially error-prone - who knows what has been set when the builder is passed around before being built? At that point, the builder itself is carrying state, which partially defeats the purpose of it generating an immutable object.

That said, I'm sure there are exceptions, though I'd have to think really hard before I'd chose a classic builder over constructors+default args or factory methods.

bidetofevil

Good incremental step

* Moving network provider tools to services * Moving network service related tests * Moving network attr init feature to the agent * Making network instrumentation depend on the agent * Initializing CurrentNetworkProvider as service * Clean up config * Created NetworkChangeInstrumentation * Updated network monitor tests * Clean up * Renaming installOn by start * Adding internal class docs * Fixing visible for tests comment * Rename .java to .kt * Removing Carrier.Builder

* Instrumentation API part 2 (#408) * Moving network provider tools to services * Moving network service related tests * Moving network attr init feature to the agent * Making network instrumentation depend on the agent * Initializing CurrentNetworkProvider as service * Clean up config * Created NetworkChangeInstrumentation * Updated network monitor tests * Clean up * Renaming installOn by start * Adding internal class docs * Fixing visible for tests comment * Rename .java to .kt * Removing Carrier.Builder * Transforming lifecycle tools to a service * Updating tests * Registering AppLifecycleService * Rename .java to .kt * Updating OpenTelemetryRumBuilderTest * Updating network change instrumentation * Adding internal comments * Reverting back to instantiating OpenTelemetryRumBuilder using the static OpenTelemetryRum.builder function * Updating tests * Using AndroidJUnit4 instead of robolectric runner

LikeTheSalad added 9 commits June 3, 2024 15:22

Moving network provider tools to services

154077a

Moving network service related tests

128be8d

Moving network attr init feature to the agent

f7b9bd7

Making network instrumentation depend on the agent

9fad4b1

Initializing CurrentNetworkProvider as service

060250c

Clean up config

c11767b

Created NetworkChangeInstrumentation

20aab44

Updated network monitor tests

8c7e9e1

Clean up

8f3bb42

LikeTheSalad requested a review from a team June 3, 2024 15:44

breedx-splk reviewed Jun 3, 2024

View reviewed changes

LikeTheSalad added 4 commits June 4, 2024 14:16

Renaming installOn by start

129dd04

Adding internal class docs

f066a2c

Fixing visible for tests comment

2f2b7b0

Rename .java to .kt

eb5ad54

breedx-splk mentioned this pull request Jun 4, 2024

Programmatically disabling instrumentation on classpath #411

Open

breedx-splk reviewed Jun 4, 2024

View reviewed changes

Removing Carrier.Builder

df79f91

breedx-splk approved these changes Jun 5, 2024

View reviewed changes

bidetofevil approved these changes Jun 6, 2024

View reviewed changes

LikeTheSalad merged commit 42635e1 into open-telemetry:feature/instrumentation-api Jun 6, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrumentation API part 2 #408

Instrumentation API part 2 #408

LikeTheSalad commented Jun 3, 2024

breedx-splk left a comment

breedx-splk Jun 3, 2024

LikeTheSalad Jun 4, 2024 •

edited

Loading

breedx-splk Jun 4, 2024

bidetofevil Jun 6, 2024

breedx-splk Jun 3, 2024

LikeTheSalad Jun 4, 2024

bidetofevil Jun 6, 2024

breedx-splk Jun 6, 2024

LikeTheSalad Jun 6, 2024

bidetofevil Jun 9, 2024

LikeTheSalad Jun 10, 2024

bidetofevil Jun 11, 2024

breedx-splk Jun 3, 2024

LikeTheSalad Jun 4, 2024

breedx-splk Jun 3, 2024

LikeTheSalad Jun 4, 2024 •

edited

Loading

breedx-splk Jun 4, 2024

breedx-splk Jun 3, 2024

breedx-splk Jun 3, 2024

LikeTheSalad Jun 4, 2024

breedx-splk Jun 4, 2024

LikeTheSalad Jun 5, 2024

bidetofevil Jun 6, 2024

bidetofevil left a comment

		NetworkAttributesSpanAppender.create(
		ServiceManager.get().getCurrentNetworkProvider());

Instrumentation API part 2 #408

Instrumentation API part 2 #408

Conversation

LikeTheSalad commented Jun 3, 2024

breedx-splk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LikeTheSalad Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LikeTheSalad Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidetofevil left a comment

Choose a reason for hiding this comment

LikeTheSalad Jun 4, 2024 •

edited

Loading

LikeTheSalad Jun 4, 2024 •

edited

Loading