Instrumentation api change - option 1#1539
Instrumentation api change - option 1#1539LikeTheSalad wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
LikeTheSalad
force-pushed
the
inst-api-change-option1
branch
from
c92693c to
4d21ac7
Compare
| @@ -27,7 +26,7 @@ class CrashReporterInstrumentation : AndroidInstrumentation { | |||
| val crashReporter = CrashReporter(additionalExtractors) | |||
|
|
|||
| // TODO avoid using OpenTelemetrySdk methods, only use the ones from OpenTelemetry api. | |||
There was a problem hiding this comment.
todo can go away if we do this
| val openTelemetry: OpenTelemetry, | ||
| val sessionProvider: SessionProvider, | ||
| val clock: Clock, | ||
| val openTelemetry: OpenTelemetryRum, |
There was a problem hiding this comment.
nitpick: I don't love this name now -- because it suggests something from the upstream api and not a rum class. I might call it rum or openTelemetryRum.
There was a problem hiding this comment.
Makes sense. I'm up for renaming it to rum.
| attributes: Attributes = Attributes.empty(), | ||
| ) | ||
|
|
||
| fun flushLogRecords(): CompletableResultCode |
There was a problem hiding this comment.
IMO either shutdown() should achieve this functionality or we can expose forceFlush(). It feels like flushLogRecords() is probably exposed to make one particular feature work, but IMO it'd be preferable just to have one API that flushes all telemetry, if we even want to expose that at all..
There was a problem hiding this comment.
It feels like flushLogRecords() is probably exposed to make one particular feature work
That's true, it'd only be useful for crash reporting, at least for now.
shutdown() should achieve this functionality
I'm open to that option, though I would have some questions. For example, is it ok to shut down RUM from an instrumentation? Or maybe it won't be needed to do so because shutdown() should be called on a crash automatically anyway (via our shutdownHook)? - If we decide on the latter, can we ensure that the app will have enough time to get to the SDK shutdown function before the OS kills the app?
or we can expose forceFlush()
I'm open to going with a "one method to flush all signals" approach. I'd still be a bit wary about the time it'd need to take to finish executing it all before the OS kills the app on a crash, though. But it sounds better than relying on the shutdown function, because forceFlush would be called directly from the instrumentation.
There was a problem hiding this comment.
After thinking about this some more on the other PR, I would lean towards just not exposing this as a public API for now (and perhaps forever). It could be an implementation detail of the SDK: #1540 (review)
|
|
||
| val clock: Clock | ||
|
|
||
| val sessionProvider: SessionProvider |
There was a problem hiding this comment.
Happy with this but might be worth thinking about whether SessionProvider.noop() should still be exposed. Will end-users actually invoke that function?
There was a problem hiding this comment.
Good point. I guess it might be needed in case they wanted to create their own implementation of OpenTelemetryRum without sessions? It sounds like an edge case, though, so maybe it's still fine to remove it.
| import io.opentelemetry.sdk.common.Clock | ||
| import io.opentelemetry.android.OpenTelemetryRum | ||
|
|
||
| class InstallationContext( |
There was a problem hiding this comment.
We could perhaps consider whether InstallationContext is required anymore given that Context and OpenTelemetryRum could plausibly be passed into the install/uninstall functions instead.
If we do keep it, perhaps it's worth renaming to something like InstallationParams given that Context has a special meaning on Android already, and that OTel already has a separate concept of context.
There was a problem hiding this comment.
I don't have a strong opinion on this, so I'm ok with either removing it or renaming it 👍
| import io.opentelemetry.android.OpenTelemetryRum | ||
|
|
||
| class InstallationContext( | ||
| val context: Context, |
There was a problem hiding this comment.
I definitely like the fact that there are only 2 params left over here now.
3 participants
DO NOT MERGE
This is a proposal for this issue: #1541 - It's meant to highlight the key changes, without addressing potential compilation errors, if any. If chosen as the approach we'd like to go with, then I'll add the rest of the changes needed for the build to pass.