Refactor session handling #564
Conversation
| return session.getId() | ||
| } | ||
|
|
||
| private fun sessionHasExpired(): Boolean { |
There was a problem hiding this comment.
Without a timer, session will expire only when getSessionId is called. I think the only implication is that the session.end event may not be accurate.
There was a problem hiding this comment.
It's true. This is how it was built originally in the Splunk codebase. I'm not certain we will want to continue doing that long term, but it's what we have right now.
As far as "accuracy" goes, it's probably accurate (meaning that the session isn't actually ended until the get() is called) -- but it could be longer than the specified timeout, which is confusing to users.
There was a problem hiding this comment.
👍 This is probably a detail that can be handled by customizing the implementation (if it matters to some users).
There was a problem hiding this comment.
Thanks! I think it's a good start, I have a couple of comments that don't need to be addressed now. Although there's just one thing that doesn't seem like it belongs in this refactoring PR which is that we're also adding a new feature, which is to send the start and end events. If possible I'd like to address that new feature in a separate PR to properly focus on its details later.
core/src/main/java/io/opentelemetry/android/SessionIdEventSender.kt
Outdated
Show resolved
Hide resolved
| @@ -76,6 +77,7 @@ public final class OpenTelemetryRumBuilder { | |||
| private final OtelRumConfig config; | |||
| private final List<AndroidInstrumentation> instrumentations = new ArrayList<>(); | |||
| private final List<Consumer<OpenTelemetrySdk>> otelSdkReadyListeners = new ArrayList<>(); | |||
| private final SessionIdTimeoutHandler timeoutHandler; | |||
There was a problem hiding this comment.
I think this is fine for the purpose of replacing the existing behavior's implementation with the new API. However, since this is the core part of the lib, I think it should receive the lowest level part of the API, which is SessionProvider. Passing a SessionIdTimeoutHandler in core implies that the only way all apps can get their session handled is by time, even if that doesn't match their specific requirements. Probably a good place to pass SessionIdTimeoutHandler would be in the higher-level type AndroidAgentBuilder that it's proposed here. That way the "time-based" approach that we recommend would be the easiest to set up via the agent, though it would still be possible to override it when needed.
There was a problem hiding this comment.
In fact, probably only SessionProvider should live in core and we should move all of its subtypes/helpers over to android-agent.
There was a problem hiding this comment.
We can talk more about this, because I think that we should be opinionated about sessions having a default max time to live...but the user should ultimately be able to configure and/or override this. As you pointed out, it's also consistent with the existing/current behavior.
Can we work on this aspect in a follow-up PR effort?
There was a problem hiding this comment.
Sounds good. We can follow up on it later 👍
|
Ripped out the event sending observer, so this is more of a "pure" refactoring. Ready for another look. |
This is a refactoring that aligns our session handling code more closely with the js session work. See this issue for the complete context: open-telemetry/opentelemetry-js-contrib#2358... especially the diagram. This doesn't match it 100% but it's close.
The
SessionIdclass previously encapsulated the session id itself, and has been largely refactored into theSessionManageryou see here. Thesession.startandsession.endevents are now wired, replacing the session change event.I also took some opportunities to convert a few things to kotlin along the way.
Differences/Gaps/Shortcomings:
onSessionStarted()takes both the new session and the old session, so that they can be more easily stitched (see thesession.startevent in semconv, for example). I'll recommend this to @martinkuba as well.SessionStorageexists, but there is only the in-memory version right now.SessionManageris likely not thread-safe.