open-telemetry / opentelemetry-android

OpenTelemetry Tooling for Android
Apache License 2.0
146 stars 34 forks source link

"AppStart" instrumentation span is never started. #560

Open beekhc opened 1 month ago

beekhc commented 1 month ago

I'm using ActivityLifecycleInstrumentation, but never getting "AppStart" events. It's possible that I'm using the instrumentation wrong, but looking through the code, it looks like nothing ever calls AppStartupTimer#start(Tracer). It's a public method, but startupTimer is a private member in ActivityLifecycleInstrumentation and there's no setter, so I don't see any way to call it manually, either.

To reproduce the issue, use the ActivityLifecycleInstrumentation in an example app and observe that there are no "AppStart" events. Alternatively, set a breakpoint in AppStartupTimer#start(Tracer) and run in debug mode and observe that the function is never called.

I'd be happy to contribute the PR implementing the call, but it's not obvious to me where it should be called. My best guess is that it should be called in ActivityLifecycleInstrumentation#install, since that's the earliest point we can get a tracer. Admittedly, that would miss out on the time spent before OTel was initialized.

LikeTheSalad commented 1 month ago

Hi @beekhc

Thank you for taking the time! I just confirmed that we're not calling AppStartupTimer#start(Tracer) which is wrong, so I agree that we should call it in ActivityLifecycleInstrumentation#install since that's the earliest we can get a tracer for it. If you're interested in creating a PR for it it would be very helpful for us, so I'd say go ahead! :)

What you mentioned about the time ActivityLifecycleInstrumentation#install is currently called not being the earliest we should start the timer is a very valid point, and I also believe this event shouldn't be a span as it is now, so I think we need to work on a refactor in the future to do it properly, i.e. starting the timer as soon as possible and sending an event instead of a span. For now, our priority is mostly focusing on the API foundations so that we can mark it stable as soon as possible, and I believe that once that's done we can start focusing on adding more features and enhancing the existing ones. So in the meantime, I think it's fine to keep the span and start it in ActivityLifecycleInstrumentation#install.

I'll loop @breedx-splk in just in case I missed something.

breedx-splk commented 1 month ago

I agree with what @LikeTheSalad said, and I think this is probably a bug/shortcoming....however @beekhc you didn't specify if you were attempting to use the instrumentation alone (without the agent) or with the agent? While we sure may want the instrumentation to be usable alone, our focus right now is definitely to get it working first within the context of the agent. 👍🏻

beekhc commented 1 month ago

Thanks for the quick response. Makes sense. I'll send a pull request next week.

@breedx-splk I think I am using it with the agent, but it's possible I have the terminology wrong. I'm configuring OpenTelemetryRum like this:

            val rumConfig = OtelRumConfig()

            val anrInstrumentation = AnrInstrumentation()
            val crashInstrumentation = CrashReporterInstrumentation()
            val lifecycleInstrumentation = ActivityLifecycleInstrumentation()
            val slowRenderingInstrumentation = SlowRenderingInstrumentation()

            val otelRum = OpenTelemetryRum.builder(app, rumConfig)
                .setResource(resource)
                .addSpanExporterCustomizer {
                    traceExporter
                }
                .addMeterProviderCustomizer { builder, _ ->
                    builder.setResource(resource)
                    builder.registerMetricReader(
                        PeriodicMetricReader.builder(metricsExporter).build()
                    )
                }
                .addLoggerProviderCustomizer { builder, _ ->
                    builder.setResource(resource)
                    builder.addLogRecordProcessor(SimpleLogRecordProcessor.create(logsExporter))
                }
                .addInstrumentation(anrInstrumentation)
                .addInstrumentation(crashInstrumentation)
                .addInstrumentation(lifecycleInstrumentation)
                .addInstrumentation(slowRenderingInstrumentation)
                .build()

I'm just starting to play with OTel on Android, so I'm not sure if that's the recommended way to do it, but everything other than AppStart seems to be working as intended.