Enable session-id based probabilistic sampling via OpenTelemetryRumBuilder.

[breedx-splk]

Here's an alternative to #971. This updates the SessionManagerImpl to ensure that the session is always stored in the otel context, This uses a new SessionObserver to track the Session, and it creates a new opt-in method on the OpenTelemetryRumBuilder called enableSessionBasedSampling(ratio).

This new method wires up an instance of the SessionIdRatioBasedSampler via the existing addTracerProviderCustomizer() mechanism.

There's currently no real way to sample logs, so there's a placeholder/TODO to add that later.

[GerardPaligot]

The class SessionFromContextProvider is pretty much explicit about where we are fetching the current session but not this method. If we want to continue to use a custom SessionManager (with setSessionManager) 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.

[GerardPaligot]

Another proposition without the need of a context and allow us to provide a custom SessionManager if it is require by the consumer.

    public OpenTelemetryRumBuilder enableSessionBasedSampling(double ratio) {
        return enableSessionBasedSampling(ratio, SessionManagerImpl.create());
    }

    public OpenTelemetryRumBuilder enableSessionBasedSampling(double ratio, SessionManager sessionManager) {
        Sampler sampler = buildSampler(ratio, sessionManager);
        addTracerProviderCustomizer((builder, application) -> builder.setSampler(sampler));

        // TODO: When there is a mechanism for sampling logs, we need to wire
        // it up here...

        this.sessionManager = sessionManager;

        return this;
    }

    private static Sampler buildSampler(double ratio, SessionProvider sessionProvider) {
        if (ratio < 0.0 || ratio > 1.0) {
            throw new IllegalArgumentException("ratio must be in range [0.0, 1.0]");
        }
        if (ratio == 0.0) {
            return Sampler.alwaysOff();
        }
        if (ratio == 1.0) {
            return Sampler.alwaysOn();
        }
        return new SessionIdRatioBasedSampler(ratio, sessionProvider);
    }

[breedx-splk]

The class SessionFromContextProvider is pretty much explicit about where we are fetching the current session but not this method. If we want to continue to use a custom SessionManager (with setSessionManager) 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.

Yes, this is a really good point -- and I really do wish that we didn't have SessionManager as a pluggable component.

This also makes me realize that we have a loose contract around the observers as well -- the interface has addObserver() through the SessionPublisher interface (which SessionManager extends), but there's nothing to require a SessionManager implementation 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.

[breedx-splk]

I really liked the idea of "just store the session in the context" when I first heard it. I thought it was a super interesting thing. Now after having implemented it and gathering some feedback, I think isn't viable....and I'll now attempt to summarize why before closing this out:

1. There isn't one context...at least in Java. The idea of "store the session in the otel context" should be met with the question of "which otel context"? The "current" context changes all the time, depending on several factors, not the least of which is thread context and span context. And the root context (and all children of it) are all immutable...which makes storing something "global" like the session kind of moot, as it's impossible.
2. Having two places to get the session id from is an anti-pattern IMO. If code sometimes consults the SessionManager and sometimes it reads it from the context, then this is inconsistent. In fact, being able to read the session from the context bypasses the current mechanism that only changes the session id on read...effectively breaking the timeout mechanism that is initiated on read (get).
3. Having a user-providable SessionManager interface means that none of the SessionManager behaviors can be assumed...and I hate this. There should just be one session manager, and it should opinionated...this would make all of our lives easier.

Thanks for playing out this exercise with me!

For now, I'm going to close this with the intention of replacing the Context bit with an observer and see how that's received.

[breedx-splk]

Another attempt that uses a SessionObserver that follows the session instead of relying on the Context.

[GerardPaligot]

While I still have some questions about adopting the observer pattern in this contribution, I prefer this approach over the previous one that relied on the OpenTelemetry context.

[GerardPaligot]

From what I understand, this class was introduced due to the belief that:

Having a user-providable SessionManager interface means that none of the SessionManager behaviors can be assumed...and I hate this. There should just be one session manager, and it should opinionated...this would make all of our lives easier.

However, I'm not fully convinced this is actually a problem. The current SessionManager already extends both SessionProvider and SessionPublisher, which together define a clear contract. Any implementation (whether user-provided or internal) must satisfy this contract. While the observer pattern might make sense for external libraries that shouldn't depend directly on the OpenTelemetry Android sdk, I’m not sure it’s the best fit within the sdk itself.

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.

[LikeTheSalad]

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.

[LikeTheSalad]

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.

[bidetofevil]

Given that #979 solves this use case and others, I think I prefer that to this. If y'all decide go narrow the scope, this is OK for me too.

[bidetofevil]

LGTM. It adds a bit of surface area to the API but it's kind of necessary if we are offering sessionId based sampling as an option.

[breedx-splk]

Closing for now, in favor of #979 -- which solves the sampler problem by allowing the user to inject their own session provider....which means they can create their own instance of the existing SessionIdRatioBasedSampler.