-
Notifications
You must be signed in to change notification settings - Fork 80
Session Management Infrastructure #1420
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
Session Management Infrastructure #1420
Conversation
Squashed commits: [749a138] split up the metrics exporter adapter interface and concrete class into tow separate files [1156525] addressed warnings [1ebaf46] added the incubating annotation, added documentation, and reordered the metrics exporters to ensure the ids are applied prior to writing the metrics to disk to ensure the right ids are set [c137908] removed redundant comments [097eef5] spotless apply updates [7e9fee9] added infrastructure to set the ids for metrics as well [a582f73] Reduced the repeated code amongst the code to set the session identifiers [9ddf60f] api and spotless apply updates [907e06e] added trim to the previous session id before the empty check and reduced a condition for adding attributes [e98ffc5] added documetnation and integrated the log record builder with setting the session identifiers [974f26f] enabled consistent session ID handling across spans and log record with the appenders [260d3c3] added SessionIdFacade pattern with UUID fallback, session extensions for telemetry types, and test coverage [df2c13d] fixed SessionManager concurrency
Fixes detekt UnusedImports warnings in: - MetricDataTypeFactory.kt - PointDataFactory.kt - SessionMetricDataTypeFactory.kt Signed-off-by: Gregory-Rasmussen_cvsh <[email protected]>
01506ff to
e4b2def
Compare
Add missing getPreviousSessionId() method to SessionProvider mock in SdkPreconfiguredRumBuilderTest to match updated interface. Signed-off-by: Gregory-Rasmussen_cvsh <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1420 +/- ##
==========================================
+ Coverage 63.65% 65.46% +1.80%
==========================================
Files 159 180 +21
Lines 3134 3353 +219
Branches 326 341 +15
==========================================
+ Hits 1995 2195 +200
- Misses 1042 1058 +16
- Partials 97 100 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for opening these PRs @gregory-at-cvs - it will be really great to improve the session management. My initial instinct is that this and the other PR have a very large diff that will make it hard to review the code thoroughly. @breedx-splk do we have any sort of official/unofficial policy for acceptable PR size on this project? My usual benchmark is <500 LOC changes. Would be interested to hear your thoughts and whether it's worth making any implicit rules explicit in the contributing guide. |
…are used as metrics
You're welcome and thanks @fractalwrench. @fractalwrench and @breedx-splk For this project, do you have guidelines on the number of LoC for different types of changes: architectural, wide breadth or far reaching, new features, or bug fixes? I'll take a look at ways to break down this PR and the integration PR more into smaller sets of changes. I'll get back to you on this. |
Thanks thanks @gregory-at-cvs and @fractalwrench . I agree that this PR is likely too large to meaningfully review. I added some new text to the |
breedx-splk
left a comment
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.
Putting a preemptive change request here, based on size and approach.
The structure and content of the PR description lead me to believe that this PR was likely created with AI tooling, which I think would be helpful to disclose with full transparency. There is an OpenTelemetry policy about using these tools here: https://github.com/open-telemetry/community/blob/main/policies/genai.md TL;DR is that using AI tools is allowable, but I think it can be informative for reviewers to know ahead of time.
In addition to being too big, I am quite concerned about the approach presented here. We certainly don't want to create new data classes to hold existing telemetry data along with session information. Please review the current semantic convention for session here: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/session.md and note that the session.id and session.previous_id are both otel attributes -- attributes which can/will be present on spans and events. You wouldn't want to put session id on metric data points, because that would make them very high cardinality indeed.
Thank you for your feedback, @breedx-splk and @fractalwrench . Regarding the AI usage, thanks for the policy link. Regarding the PR sizes, I broke down my original changes into the three PRs and will continue to do so now that I know your preferences. That makes sense. I can break it up by adding the changes to separate PRs for:
If there is a different breakdown that you all would prefer or if you would like to discuss more, let’s chat. Regarding the metrics approach, this is very helpful feedback. The opt-in approach addresses cardinality concerns by keeping it disabled by default. If you would prefer to completely remove the metrics implementation, I will do so. Which direction best works for OpenTelemetry? Thanks again. P.S. If all you are ever curious, here is my personal OSS project, which shows my work from architectural design and down to the code: Shuttle. |
|
I will break down this PR into smaller ones. |
|
That approach for separate PRs sounds great. I don't have a preference for metrics as tbh I'm not that familiar with that part of OTel - @breedx-splk probably has more opinions than me. |
PR #2: Session Management Infrastructure
Description
Adds foundational infrastructure for session tracking across all telemetry signals.
This PR includes:
Core Session Utilities
SessionIdentifiers- Data class for session.id and session.previous_idSessionExtensions- Kotlin extensions for adding session IDs to spans/logs/metricsSessionIdentifierFacade- Facade pattern for consistent session accessSessionIdFacade- UUID fallback for session ID generationMetrics Infrastructure
SessionMetricExporterAdapter- Adapter pattern for adding session IDs to metricsKey Addition: This enables session-based metrics analytics - a capability
not present in the iOS or JavaScript SDKs! This allows correlating metrics
like memory usage, network bytes, or performance counters to specific user sessions.
Design Patterns Used
Related PRs
This is part 2 of 3 in a comprehensive session management enhancement:
Dependencies:
Review Strategy:
Type of Change
Checklist
Testing
Additional Context
Comparison with Other Platforms:
This infrastructure establishes the foundation for comprehensive session tracking.
PR 3 integrates this across all instrumentation modules.
Why metrics matter: Session-based metrics enable queries like:
None of the other SDKs can answer these questions today!
References