Exposing SessionProvider setter

[LikeTheSalad]

Alternative to #974 and #971

Solves #841 and #842 (comment)

High level changes:

• Leaving only the SessionProvider as part of the session module and moving everything else to the agent module. Reason: The core module only needs the SessionProvider to get a session.id at the moment. All the other API surface for sessions is tied to our opinionated session provider implementation (the SessionManager). By reducing the API surface in session, users of core will have less code to implement, and, should they wanted to make their own SessionProvider implementation "observable" for example, they can do so by creating their own contracts that are compatible with it.
• Making SessionProvider publicly settable so that users of core can provide their implementation.
• Setting our opinionated SessionProvider implementation (the SessionManager) from within the agent initialization.

NOTE: The build is broken on purpose as I didn't address unit tests and other housekeeping to keep the changes focused on the proposal. If accepted, I'll make the changes needed to make the build pass.

[LikeTheSalad]

This will allow users of core to set their own session provider impl.

[LikeTheSalad]

The SessionManager will live only in the agent, and it will be our opinionated SessionProvider implementation.

[LikeTheSalad]

The agent will initialize and set our opinionated SessionProvider impl during its initialization.

[marandaneto]

Extracting to its package and allowing a custom setter makes sense to me
This is most likely being used only by vendors that have a different opinion on how sessions should work

[marandaneto]

The linked PRs try to add some built-in sampling configuration, which this PR does not.
Would it be the user's responsibility to implement the sampler by themselves and set their custom session handler?

[marandaneto]

if the agent depends on https://github.com/open-telemetry/opentelemetry-android/tree/main/session
should we move this package to that module?

[LikeTheSalad]

That's the case right now, most of the interfaces were moved from the session module into the agent. The one interface that remains in the session module is SessionProvider. The reasoning for it is because SessionProvider is currently the only part of the API that's needed in core, and because the rest of interfaces (SessionPublisher, SessionObserver, and so on) depend on the specifics of our implementation of SessionProvider, but are otherwise not linked to SessionProvider by its contract. I guess we can just leave the interfaces in the session module, and it won't affect the goal of this PR tbh, though when it comes to API surface, I prefer to start small and expand as needed. It's also kinda strange to me that we keep implementation-specific contracts in an API package.

[bidetofevil]

LGTM.

Should we add a test that verifies that if a different sessionId is provided, the observers are sent the correct ID so that we can verify that the plumbing works as expected?

[breedx-splk]

Given that we've got 2 approvals on this, it's looking like this is an approach we want to try/experiment with, and I'm fine with that...but I do need to point out two things that might have been overlooked:

1. This PR constitutes a design change which is no longer aligned with the design we worked on last year. With no dependencies on or public usages of SessionManager, users who implement their own SessionProvider interface can do whatever they want. It is very flexible indeed, but a design change. Just calling this out, since we had worked on it previously and it was implemented in at least 2 languages. We will want to stay consistent, however that ends up looking.
2. It does not solve the use case in Impossible to apply SessionIdRatioBasedSampler after session refactoring #841, wherein a user wants to merely wants to wire up a session id (ratio-based) sampler while using the default/built-in session provider! Sure, they can build their own SessionProvider implementation, but I don't think they can get a handle to any existing implementations to wire up a SessionIdRatioBasedSampler. Womp womp.

What I do like is that it's considerably simpler, though, and has a reduced api surface.

[LikeTheSalad]

The linked PRs try to add some built-in sampling configuration, which this PR does not. Would it be the user's responsibility to implement the sampler by themselves and set their custom session handler?

Users of core will need to implement their own SessionProvider, in which they can reuse our sampler, SessionIdRatioBasedSampler. So the sampler can be the same, but you're right in that they will need to come up with their custom logic to handle sessions.

[LikeTheSalad]

Should we add a test that verifies that if a different sessionId is provided, the observers are sent the correct ID so that we can verify that the plumbing works as expected?

Sure, I'm planning to add tests and address other housekeeping tasks before merging.

[LikeTheSalad]

Given that we've got 2 approvals on this, it's looking like this is an approach we want to try/experiment with, and I'm fine with that...but I do need to point out two things that might have been overlooked:

Thanks for your feedback, @breedx-splk. Even though this PR has gotten 2 approvals, I still would like you to be on board (not necessarily approve, but at least not too concerned) with at least most of what's proposed here before merging it, given that you're a maintainer. Your feedback seems to suggest that these changes won't solve the issue that they're supposed to, and despite I believe that we should take the opportunity to make as many changes as possible while this project isn't stable, that's only valid if those changes mean an improvement over the current state, which in this case to me it would mean not only solving this issue, but other types of session related issues as well, such as the ones mentioned here and here, where some use cases demand for more user ownership about session management in general. I'll try to address your concerns below so that we can hopefully clarify how these changes can help solve those issues, and whether it makes sense to merge these changes as they are, or if we should revert some of them, as I mentioned earlier in response to one of @marandaneto's comments.

1. This PR constitutes a design change which is no longer aligned with the design we worked on last year. With no dependencies on or public usages of SessionManager, users who implement their own SessionProvider interface can do whatever they want. It is very flexible indeed, but a design change. Just calling this out, since we had worked on it previously and it was implemented in at least 2 languages. We will want to stay consistent, however that ends up looking.

The idea of moving SessionManager, along with its complementary interfaces, to the higher-level agent module is because the lower-level core module only needs SessionProvider to do its job of attaching a session id to the span and log signals. This way, users of core will have to come up only with the bare minimum code for their implementations, which is to return a session id from the getter in SessionProvider. Having to implement SessionManager with all the bells and wistles won't provide a benefit for the core module, at least not in its current state, and to me it seems like a burden when it comes to implementing it, as there might be implementations that might not need to take advantage of the publisher/observer contracts or others, since the whole implementation will be custom, so people can come up with only the mechanisms they need in a way that's easier for them to handle and implement. Therefore it seems strange to me that we keep these convenience apis around the SessionProvider when they're essentially optional for the lowest layer of this project. If we wanted to make these mechanisms mandatory so that they all have to go together, then I think that should be reflected in the api design somehow (for example by making SessionProvider implement the publisher interface and removing the SessionManager, or maybe by removing SessionProvider and only keeping the SessionManager interface as the only way for people to provide custom session handling implementation). My understanding of the current design is that it revolves around SessionProvider, and that the rest of interfaces serve as a suggested set of mechanisms that might be commonly needed to provide end users some hooks and entry points for when they don't control how the session is managed, though I might have misunderstood it, so maybe @martinkuba might be able to help clarifying it. If that's the case though, I think those convenience apis make sense in the agent module, where we manage the session and there's no way for end users to provide a custom SessionProvider implementation, so in that case, they could listen to changes we make to the session and other aspects that the convenience api enables. In any case, as I mentioned in Manoel's comment, we can roll back moving these convenience apis to the agent module if we think they're needed outside of the agent, in case I missed some potential use case for them in core. Doing so won't affect the goal of this PR so it won't be an issue, though I guess I wanted to at least make a point on why I think those don't make much sense for core.

2. It does not solve the use case in Impossible to apply SessionIdRatioBasedSampler after session refactoring #841, wherein a user wants to merely wants to wire up a session id (ratio-based) sampler while using the default/built-in session provider! Sure, they can build their own SessionProvider implementation, but I don't think they can get a handle to any existing implementations to wire up a SessionIdRatioBasedSampler. Womp womp.

One way I see that use-case achievable would be, as you suggested, by users of core creating and setting their own SessionProvider implementation. Then they can set the same sessionprovider object into a new SessionIdRatioBasedSampler that they can attach to the trace provider customizer. Going further, since our SessionIdRatioBasedSampler class only works for traces, users can also come up with some custom sampler implementation that they could attach to logs too, if needed. This definitely will mean a lot more work for core users, since essentially core won't help them much as it will be agnostic to specific use cases, as I think it should in order to be useful for a lot of them at the same time. Should we wanted to provide user-friendly support for the session sampling use case in a way that users won't have to come up with their custom session managing implementation, then one way of doing so might be by adding explicit support for it in the agent module (e.g. by adding a sessionSampleRate argument to the agent initializer for example), that way we take all the work of maintaining this specific use case, while at the same time keeping core lean and flexible enough so that some users can enable more use cases without us having to add support for all of them.

I admit that this approach of making core as plain as possible and then adding all the user friendliness along with our opinionated implementations into the agent, doesn't leave a "middle ground" for users that might just want some benefits of the agent, while having a lot of flexibility and full customization in other fronts, as core allows. The reason why I think it's a good idea is because my understanding of the target users of core is that it'll mostly be vendors that want to wrap core around their own agent/sdk. So if we made core somewhat opinionated, then we risk running into vendors asking us to either making core more flexible, or us having to add explicit support for their use cases in core, which not only adds more work on our side but it also creates another risk of some features not being required for some other users of core, which will then result in potential complains and requests for providing ways to disable some features and/or somehow being able to remove them such that they won't bring unnecessary extra size to their sdks. That's the way I see it based on the feedback we've received so far. Of course I think core needs some non-negotiable features built-in, such as disk buffering and its configuration for example, but I think it should be rare that we would need to add support for specific use-cases in core.

I hope this provides clarifications on the concerns you mentioned above, @breedx-splk. Again, I wouldn't want to merge these changes if you don't think they provide an improvement over what we currently have, or if my interpretation of our goals with this project don't match the ones you have in mind, should that be the case then I think we need to discuss it further.

[breedx-splk]

I actually very much like that this is loosening the type -- because SessionManager isA SessionProvider, it should have been this all along!

[breedx-splk]

@LikeTheSalad thanks for the thorough and thought-out response. 💟 Appreciate that.

I think it's fine to move the non-essential session pieces into the agent and to hide them and/or make them internal. I do appreciate that all the other pieces currently only rely on the SessionProvider and that will suffice. And as I said before, I like that it shrinks the API surface -- but for what we're responsible for and what users might have to implement themselves.

The downside is that it leaves the session module with just a single interface (SessionProvider). We might as well just put that into core as well I suppose.

I don't want to block this, and I completely see how this approach does provide a roundabout way to solve the "I can't wire up a session-id based sampler, help!"...but it is still a bummer that users will need to take on the heavy lifting of providing their own session handling in order to "just configure sampling".

Because it's a step forward, let's try it out. Gotta get the build passing first, tho. 😉
...we can find a way to make sampling easier on top of this in the near future.

[LikeTheSalad]

We might as well just put that into core as well I suppose.

Good point, I didn't think of it. Since it would involve removing a module I think it should probably be done in its own PR though. It also sounds like something that Hanson mentioned about moving apis too.

but it is still a bummer that users will need to take on the heavy lifting of providing their own session handling in order to "just configure sampling"

Btw my changes on the session APIs are based on the feedback we've gotten so far, plus an effort to keep our opinions mostly in the agent module. However, I'm open to reverting them or adding opinionated features to core in the future if we see that they make sense there, maybe due to changes on the specs or new user feedback that requests it at a lower level. It's easier to add stuff later rather than remove it, anyway.

Also, thank you for your patience, especially for going through that quite long comment of mine.

[LikeTheSalad]

@breedx-splk @bidetofevil @marandaneto I've just updated the code to make it compile and also fixed the unit tests. Also, I had to move back some of the session interfaces into the session module because they were being used by the SessionInstrumentation class, and I missed that part before. In any case, I know it's already approved though I wanted to let you know in case you wanted to have a look at the full picture before merging it.