Enable manual creation of screen view spans #851
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
Currently builds and runs, but crashes on navigation to the JetpackComposeActivity, presumably because of some missing runtime dependency. |
|
@breedx-splk @laurit hey! |
jtmalinowski
force-pushed
the
jetpack-compose-support
branch
4 times, most recently
from
7828651 to
c6dfded
Compare
|
I have read the CLA Document and I hereby sign the CLA |
jtmalinowski
force-pushed
the
jetpack-compose-support
branch
from
c6dfded to
053e0ec
Compare
jtmalinowski
changed the title
There was a problem hiding this comment.
I think we should hold on this for now. Once the upstream repo has finished the instrumentation refactor/reorg, we should circle back to see how we do this custom screen name setting up there.
If we absolutely need to proceed with this for some immediate thing, we must be clear that it's experimental and will break/change shortly.
|
|
||
| import io.opentelemetry.android.instrumentation.activity.VisibleScreenTracker; | ||
|
|
||
| public class ExplicitVisibleScreenNameTracker extends VisibleScreenTracker { |
There was a problem hiding this comment.
I think that this type of polymorphic inheritance can become problematic. Would much prefer to use delegation, and I think that long term perhaps VisibleScreenTracker needs to migrate to an interface (or two). I also wouldn't want to count on VisibleScreenTracker remaining an external API class for too long, things are in flux over there.
There was a problem hiding this comment.
Hm, how would you implement delegation here? For example when we construct StartTypeAwareMemorySpanBuffer it wants an instance of VisibleScreenTracker, same with ScreenAttributesSpanProcessor, right? So with the existing API I need something that extends VisibleScreenTracker anyway? I'm open to ideas.
There was a problem hiding this comment.
Okay! Moved the logic to a span processor, so we don't have the inheritance anymore.
| @@ -184,12 +188,22 @@ public LiveData<String> getSessionId() { | |||
| public void onDestroyView() { | |||
| super.onDestroyView(); | |||
| binding = null; | |||
|
|
|||
| SplunkRum.getInstance().setScreenName(null); | |||
There was a problem hiding this comment.
I think we want to keep the existing Fragments unchanged and if anything make a new one to demonstrate the behavior.
There was a problem hiding this comment.
I just added a link to my new activity, where I added Jetpack Compose Navigation, so I think this is now addressed.
| @@ -86,7 +88,7 @@ class RumInitializer { | |||
| } | |||
|
|
|||
| SplunkRum initialize(Looper mainLooper) { | |||
| VisibleScreenTracker visibleScreenTracker = new VisibleScreenTracker(); | |||
| ExplicitVisibleScreenNameTracker visibleScreenTracker = new ExplicitVisibleScreenNameTracker(); | |||
There was a problem hiding this comment.
This is also something that will need to change, based on divergence from the upstream base code. The VisibleScreenTracker is also created by the OpenTelemetryRumBuilder, so we'd need to figure out how to NOT have two instances in play.
There was a problem hiding this comment.
Definitely! For now the only usage, ie. https://github.com/open-telemetry/opentelemetry-android/blob/main/android-agent/src/main/java/io/opentelemetry/android/OpenTelemetryRumBuilder.java#L484 is addressed, because we replace their ScreenAttributesSpanProcessor with our own.
In browser RUM we had I think it's fair to assume that this functionality might be unavailable in some versions as OTel is being restructured, I'll rebuild it when we know the new API. Perhaps even OTel itself will have something for Jetpack Compose in the near future. So overall I think it's a good compromise, where customers are able to instrument Jetpack Compose via a workaround and under the new API we'll look at it again. |
|
I managed to configure the kotlin compiler after all, so the example is an actual Jetpack Compose Activity. |
| @@ -113,6 +117,23 @@ static SplunkRum initialize(SplunkRumBuilder builder, Application application) { | |||
| return INSTANCE; | |||
| } | |||
|
|
|||
| public void experimentalSetScreenName(String screenName, String spanType) { | |||
There was a problem hiding this comment.
These two new public API methods needs javadoc so that users can get a better idea about what they do and when they should/shouldn't use these.
Co-authored-by: jason plumb <[email protected]>
No description provided.