-
Notifications
You must be signed in to change notification settings - Fork 100
Enable session-id based probabilistic sampling via OpenTelemetryRumBuilder. #974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6f747ce
b54351d
4190ba6
b9863e5
2c83756
7e0f0ec
c147fce
baceb6e
86b4ffb
87ab8d1
c88b894
ba04c98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.android.internal.session | ||
|
|
||
| import io.opentelemetry.android.session.Session | ||
| import io.opentelemetry.android.session.SessionObserver | ||
| import io.opentelemetry.android.session.SessionProvider | ||
| import java.util.concurrent.atomic.AtomicReference | ||
|
|
||
| /** | ||
| * A SessionObserver that listens for session start events, stores the session, | ||
| * and is also a SessionProvider, that can return the session id. | ||
| */ | ||
| internal class SessionObservingSessionProvider : | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I understand, this class was introduced due to the belief that:
However, I'm not fully convinced this is actually a problem. The current The more I think about it, the more I believe that the SDK may not need to own the concept of a session at all. On Android, sessions are often used beyond telemetry (for example, for analytics or app-specific behaviors). In my own experience, I work on a kiosk application that resets itself after a configurable timeout, triggering a new session. In such scenarios, having a centralized, user-defined session manager becomes critical, as it allows multiple tools (including OpenTelemetry) to share a consistent session lifecycle. If that's the case, then perhaps the SDK should act only as a consumer of session data rather than its owner. In this context, adopting an observer pattern makes more sense. But to fully embrace that model, maybe all session-related logic should be removed from the core sdk, with the option for users to plug in either a custom or provided basic implementation. Just my two cents, curious to hear yours.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I see what you mean. In this case, core is being opinionated about what a session must be. Based on the project structure that I mentioned here, I think one approach we could take to avoid it, is to move our opinionated implementation of session manager from core into the agent, and then allow the interface to be set in core. That way, endusers will still have our opinionated version of a session, while other SDKs and/or expert users can define their own within core. Generally speaking, I think core should be as flexible and opinionated-less as possible, and if there's something that we don't want users to do with core, then we can work on what the interfaces allow them to do. It's a tricky balance to ensure that something is flexible enough without causing people to shoot themselves in the foot, and most likely we won't get it perfectly on the first attempt, though I think this project is in a state (not stable) that allows us to experiment with the API until we find a good balance.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've created this PR with the approach that I was mentioning in yesterday's SIG meeting, @breedx-splk, please have a look to see what you think. I think it should address both @GerardPaligot's issue and also this other one and it will also simplify the core API surface. |
||
| SessionProvider, | ||
| SessionObserver { | ||
| private val session = AtomicReference<Session>(Session.NONE) | ||
|
|
||
| override fun getSessionId(): String = session.get().getId() | ||
|
|
||
| override fun onSessionStarted( | ||
| newSession: Session, | ||
| previousSession: Session, | ||
| ) = session.set(newSession) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class
SessionFromContextProvideris pretty much explicit about where we are fetching the current session but not this method. If we want to continue to use a customSessionManager(withsetSessionManager) and use this method to apply a sampling, we need to know that we should save our session id in the context with a specific context key. Hard to know for someone that has limited knowledge about the internal implementation of this project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another proposition without the need of a context and allow us to provide a custom
SessionManagerif it is require by the consumer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a really good point -- and I really do wish that we didn't have
SessionManageras a pluggable component.This also makes me realize that we have a loose contract around the observers as well -- the interface has
addObserver()through theSessionPublisherinterface (whichSessionManagerextends), but there's nothing to require aSessionManagerimplementation to notify its observers when the session changes. It's extra knowledge, and that stinks.I don't mind keeping the session id format/value user-suppliable, but making the entire thing pluggable is turning into a headache.