Bug2020962 sessions#3438
Open
travis79 wants to merge 8 commits intomozilla:mainfrom
Open
Conversation
travis79
force-pushed
the
Bug2020962-Sessions
branch
from
7471f21 to
00f8341
Compare
badboy
reviewed
Apr 28, 2026
Member
badboy
left a comment
badboy
left a comment
There was a problem hiding this comment.
I gave this a first somewhat-thorough review and left lots of comments.
High-level I think this is a good approach, some clarifications in the details needed maybe.
I have not yet gone through all the new test; I'd like to do that before I'm out tomorrow.
What's currently missing though is docs, both dev docs about all the new state we're storing (and the state changes! oh is it time for a new diagram?), and user-facing docs.
Fine if they land in followups, but should be there before we get this out to applications.
| @@ -83,6 +83,9 @@ pub fn metric_dispatcher_benchmark(c: &mut Criterion) { | |||
| ping_schedule: Default::default(), | |||
Member
There was a problem hiding this comment.
Adding a note to the very first line of the diff for the first commit, because one still can't leave a comment for the whole commit (or the message).
Thanks for splitting it up into individual commits. That's certainly helpful, though unfortunately this commit is not working on its own, introducing use of the SessionMode before it's defined.
I think I can cope with it in review though, just comments might be a bit out of order then.
travis79
force-pushed
the
Bug2020962-Sessions
branch
from
73c309a to
a62b6a6
Compare
Introduce the `out_of_session` flag on `CommonMetricData` (defaulting to `true`) so metrics can declare whether they participate in session-level sampling. Add session configuration fields (`session_mode`, `session_sample_rate`, `session_inactivity_timeout_ms`) to `InternalConfiguration` and the RLB `Configuration` struct, and add `session_sample_rate` to `RemoteSettingsConfig` for server-side overrides. Update all existing test, benchmark, and example call sites to supply the new configuration fields.
Implement the session management engine for Glean:
- New `session` module with `SessionManager`, `SessionMode` (Auto,
Lifecycle, Manual), `SessionState`, `SessionMetadata`, and
`EventSessionContext` types, plus persistence helpers for session_id,
session_seq, event_seq, inactive_since, and session_start_time.
- Core lifecycle integration in `Glean`: session_start/session_end,
handle_client_active/inactive transitions, inactivity timeout
evaluation, dirty-flag crash recovery emitting synthetic
session_end("abnormal"/"abnormal_inactive"), orphaned session cleanup
on mode-mismatch across builds (session_end("abandoned")), and
session state restoration across clean restarts in AUTO mode.
- Boundary events: `glean.session_start` and `glean.session_end` defined
in metrics.yaml and emitted as out-of-session events carrying
session_id, session_seq, session_start_time, and sampled_in.
- Session sampling gate in `MetricType::should_record()` suppresses
in-session metrics when the active session is sampled out, applying
uniformly to all metric types.
- Event-session integration: `EventSessionContext` flows through
`event_database::record()` so in-session events carry per-event
session metadata (session_id, session_seq, event_seq,
session_sample_rate, session_start_time).
- Remote Settings override for session_sample_rate is intentionally
in-memory only; RS refreshes on every startup so persistence would
risk making stale values sticky.
- session_start_time uses millisecond-precision RFC 3339 serialization
for consistency with millisecond event timestamps.
Add session lifecycle methods to the UniFFI interface definition and wire them through all language bindings: - UDL: export SessionMode enum, session_start(), session_end(), handle_client_active/inactive session hooks, and apply_server_knobs_config session_sample_rate support. - Android/Kotlin: forward handle_client_active/inactive calls through the Glean singleton; update test configuration for session fields. - iOS/Swift: forward handle_client_active/inactive calls; update test helpers and assertions for session-aware event payloads. - Python: forward handle_client_active/inactive calls; update _RecordingUploader to accept a list of filter strings (needed because session lifecycle now submits additional pings on inactive); update test configuration. - RLB: expose session_start/session_end and configuration fields on the public Configuration struct.
Comprehensive test coverage for session management:
- tests/session.rs (30 tests): Auto/Lifecycle/Manual mode basics,
session_seq persistence and monotonic increase across restarts,
event_seq increment and reset semantics, event_seq continuity across
AUTO mode restarts, session metadata on in-session events,
out-of-session event bypass, sampling gate enforcement, boundary
event emission, inactivity timeout evaluation, session resumption
vs. new session on restart, session_start_time persistence,
sampled-out session resumption, sessions_seen diagnostic counter,
and mode-mismatch orphaned session cleanup.
- lib_unit_tests.rs (4 tests): dirty-flag crash recovery emitting
session_end("abnormal") for active crashes and
session_end("abnormal_inactive") for mid-shutdown crashes, no-op
when no session is persisted, and session_seq continuity after
crash recovery.
travis79
force-pushed
the
Bug2020962-Sessions
branch
from
a62b6a6 to
6da3515
Compare
This commit includes the refactoring of `out_of_session` to `in_session` to avoid the inversion
travis79
force-pushed
the
Bug2020962-Sessions
branch
from
6da3515 to
008e380
Compare
Member
Author
|
Okay, Part 1 commit of addressing change requests should cover just the refactoring to remove the inversion by changing |
Member
|
Oh can you update the PR title to be a bit more descriptive? |
badboy
reviewed
May 6, 2026
travis79
force-pushed
the
Bug2020962-Sessions
branch
from
5f5940f to
a9fdbdf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I attempted to structure the commits here to make this easier to review, so I recommend reviewing it that way rather than as a whole.