Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add session.change event #1042

Closed
wants to merge 8 commits into from
Closed

Add session.change event #1042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8b1f026
add session.change event
breedx-splk May 15, 2024
dbd78c0
add pr number
breedx-splk May 15, 2024
e621e8b
fix anchor
breedx-splk May 15, 2024
1a70aa9
remove space
breedx-splk May 15, 2024
e10d226
Merge branch 'main' into session_change_event
breedx-splk May 21, 2024
87371fd
Merge branch 'main' into session_change_event
breedx-splk May 28, 2024
a6821af
Merge branch 'main' into session_change_event
joaopgrassi May 29, 2024
002ef31
Merge branch 'main' into session_change_event
breedx-splk May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .chloggen/session_change_event.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Use this changelog template to create an entry for release notes.
#
# If your change doesn't affect end users you should instead start
# your pull request title with [chore] or use the "Skip Changelog" label.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: 'enhancement'

# The name of the area of concern in the attributes-registry, (e.g. http, cloud, db)
component: 'session'
joaopgrassi marked this conversation as resolved.
Show resolved Hide resolved

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add new experimental `session.change` event

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
# The values here must be integers.
issues: [1042]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
12 changes: 11 additions & 1 deletion docs/general/session.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ the Logs, Events, and Spans generated during the Session's lifecycle.

When a session reaches end of life, typically due to user inactivity or session timeout, a new session identifier
will be assigned. The previous session identifier may be provided by the instrumentation so that telemetry
backends can link the two sessions.
backends can link the two sessions (see [Session Change Event](#session-change-event) below).

## Attributes

Expand All @@ -35,4 +35,14 @@ backends can link the two sessions.
<!-- END AUTOGENERATED TEXT -->
<!-- endsemconv -->

## Session Change Event

![Experimental](https://img.shields.io/badge/-experimental-blue)

When a session changes (due to time-based expiry or another mechanism), an event MAY be emitted.

The emitted event that represents a session change MUST have the `event.name=session.change`.
The event body MUST be empty and the attributes MUST include both the `session.id` and `session.previous_id`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow session.id to be null to denote the change resulting in a session expiring but no new session beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. What's the use case? Session end? Seems like we could have a separate event then, keeping parity with a hypothetical session start event.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use case is when the "change" doesn't lead to a new session - just the ending of the previous one, like when there is a requirement for a new session that is not fulfilled.

For instance, if a session times out, but the app is in the background, and no telemetry will be recorded while that is the case, do we want to generate a new session.id?

An alternative is to use some non-null stub to indicate that there's no session, e.g. empty-string. If we go down that route, we should define what that non-null stub should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for clarifying. I think I better understand now where you are coming from.

What this feels like is that both @bidetofevil and @scheler are suggesting changes to this event in order to allow 3 different semantics within one single event: "session start", "session change", and "session end". In order to facilitate this, you've suggested changes that, in my opinion, complicate things.

By allowing either field to be null/absent, the receiver then needs to check these states:

  • both present? then it's a change event
  • just previous absent? then it's a start event
  • just session id absent? then it's an end event

and then we would also require wording to require at least one of them, so that both can't be blank.

I'd prefer not to mess with any of that and just have clear, simple, single-semantic events. So if/when we need a session start event, let's spec that separately. Similary, a session-end event that has no successive session could also be spec'd later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with having separated events for single cases like session change/end/start.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds good. Breaking that out makes it clear what a backend should do when they receive one of those events. These are 3 different state changes so having separate events make sense.

attributes, as described above. The values MUST be different.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording session.change indicates there is another event to represent when a new session starts. However, there is no such event currently. I suggest we call this session.start or just session, and this event MUST include session.id and MAY include session.previous_id when applicable.

Structurally, this event should be described using the Events format. See https://github.com/open-telemetry/semantic-conventions/blob/main/docs/mobile/events.md for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording session.change indicates there is another event to represent when a new session starts.

That's a bit of a stretch to me, but is probably highlighting the fact that a larger spec around session handling might be missing. It's perfectly reasonable to also have other telemetry include the session without a session start event, IMO.

However, there is no such event currently. I suggest we call this session.start or just session, and this event MUST include session.id and MAY include session.previous_id when applicable.

Yeah, I think this demonstrates that what you're suggesting is semantically something different than what I was trying to define here.

Structurally, this event should be described using the Events format. See https://github.com/open-telemetry/semantic-conventions/blob/main/docs/mobile/events.md for an example.

Agreed, however there's no precedent for an empty body. Maybe you're suggesting that this event should have those attributes as body fields instead?

Copy link

@bidetofevil bidetofevil May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there's no notion of a session right now beyond that all signals that contain the same session.id attribute with a given value conceptually belong to the same session, having this to signal that the previously set session.id is no longer valid seems reasonable to have even if there's no equivalent session.start event.

The latter would be useful if there was additional metadata attached to said session, but in the absence of that, I don't think we necessarily need that. Or if we do, it should be part of a different proposal to define what a session is and how it relates to signals.

Also, +1 on making this an Event and have these be defined as body fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I'm not opposed to using body fields, but if we have previously standardized on "session.id is an attribute that gets slapped onto all client telemetry, we either need to make an exception for this specific event (eg. "The session attribute isn't here because it's in the body") or we need to exclude it from the body, which means there's just the one session.previous.id` in the body).

I thought both of these options were more confusing than just having attributes.

Copy link
Member

@joaopgrassi joaopgrassi May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if what I will say is completely dumb, but reading the changes and the discussions here I'm thinking: What if we only have session.start and session.end? And to signal a "change in session", we emit a session.end and a session.start having a session.previous_id? Then when a new start event doesn't have the session.previous_id we know it's a brand new session, and when we have the attribute it indicates a change. Was this option discussed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't discussed. That approach sounds viable as well, although it has two minor concerns for me, neither of which is a deal-breaker:

  1. there is a bit of redundancy in the "session change" case -- that is, an end and and start are both always emitted, while the presence of session.previous_id in the start case already suggests that the prior session is ending (or has just ended)
  2. does the event receiver then have to pay slightly more attention to event ordering? If a session change results in a stop/start sequence that is out of order (start/stop), is that MUCH harder to handle (or require keeping state?). I don't know if it's a problem, but I know enough to be cautious about that choice.

In any case, it seems that folks are thinking more in terms of start/stop than "change", so I'll mark this one draft and write up another PR that tries this. Thanks for the suggestions folks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I'm not opposed to using body fields, but if we have previously standardized on "session.id is an attribute that gets slapped onto all client telemetry

Perhaps we are overloading the meaning of session.id then. The one in attributes indicates what session this event belongs to, while for the proposed body field, it's the NEW sessionId being introduced - so it might make more sense as session.new_id or something?

FWIW, I also agree that making this event "atomic" to express both the end and beginning at the same time is a good idea, in case the telemetry comes in out of order, or just delayed, which will happen pretty frequently).


[DocumentStatus]: https://github.com/open-telemetry/opentelemetry-specification/tree/v1.33.0/specification/document-status.md
Loading