chore: added session timeout minutes config in OtelConfig #330
Conversation
|
Hey thanks @Archish27. Take note that the build is currently broken and you should run |
android-agent/src/main/java/io/opentelemetry/android/SessionIdTimeoutHandler.java
Outdated
Show resolved
Hide resolved
android-agent/src/main/java/io/opentelemetry/android/SessionIdTimeoutHandler.java
Outdated
Show resolved
Hide resolved
android-agent/src/main/java/io/opentelemetry/android/config/OtelRumConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the contribution @Archish27...I think this approach is solid, but I have noted a few changes that I think need to be made. Furthermore, the SdkPreconfiguredRumBuilder will also need to be modified to take the config and pass the configured timeout to the SessionIdTimeoutHandler that it creates.
Both of these cases are violations of the dependency injection principle, so maybe a better change is to have the SessionId passed into the constructors.
Thanks again!
I agree, just to provide a bit more details, I think the steps to address this could be:
|
Hey @LikeTheSalad SessionId seems to be getting exposed outside classes is the expected behavior? Also, along with SessionTimeoutHandler will get exposed too. Can you suggest what can we do here? 
|
Good point, we still haven't fully figured out what will be part of the public and/or internal classes so I don't have a clear answer here, although I think we can just expose them for now, taking into account that this project is not stable yet. And then we can re-evaluate this later once we have all the features we need for the first stable version and see if this makes sense as a public class or not within that state of the project. Otherwise, I think that slowing down development over these situations while the project is not stable would defeat the purpose of having it marked as not-stable right now. Any objections/thoughts on this @breedx-splk ? |
| this(Clock.getDefault(), TimeUnit.MINUTES.toNanos(15)); | ||
| } | ||
|
|
||
| SessionIdTimeoutHandler(long sessionTimeoutMinutes) { |
There was a problem hiding this comment.
Any reason we don't use Duration here as well to make as more flexible? A granularity of minutes seems a bit coarse for general usage. Doing that will also mean we don't have to add a separate method just for testing, reducing the API surface.
There was a problem hiding this comment.
Agreed, cleaner to use Duration here as well.
Yeah, I was surprised that
No objections, I think I agree. Fixing this is definitely not in scope for this PR, which I think is getting pretty close to being ready. Long-term, I think the right approach is to make |
…etry#330) * chore: added session timeout minutes config in OtelConfig * chore: added custom timeout in otel config * chore: Formatting fixes
Fixes : #329