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.

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

Add session.change event #1042

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

breedx-splk
Copy link
Contributor

@breedx-splk breedx-splk commented May 15, 2024

Changes

This adds semantic conventions for an event called session.change.

Merge requirement checklist

  • CONTRIBUTING.md guidelines followed.
  • Change log entry added, according to the guidelines in When to add a changelog entry.
    • If your PR does not need a change log, start the PR title with [chore]
  • schema-next.yaml updated with changes to existing conventions. (I think this is N/A due to automation lacking for events)

@breedx-splk breedx-splk requested review from a team as code owners May 15, 2024 22:18

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`
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

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

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.

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).

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`

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.

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.

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.

@breedx-splk breedx-splk marked this pull request as draft May 29, 2024 16:05
@breedx-splk breedx-splk marked this pull request as draft May 29, 2024 16:05
@breedx-splk breedx-splk mentioned this pull request May 29, 2024
3 tasks
Copy link

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

I think I still prefer making session.next_id and session.previous_id body fields and reserving session.id to denote what the session the event actually belongs to, so the meaning of session.id is not overloaded.

That said, I understand wanting to minimize duplication, so I'll leave it up to you to make the call after saying my piece.

@breedx-splk
Copy link
Contributor Author

I think I still prefer making session.next_id and session.previous_id body fields and reserving session.id to denote what the session the event actually belongs to, so the meaning of session.id is not overloaded.

That said, I understand wanting to minimize duplication, so I'll leave it up to you to make the call after saying my piece.

Yeah, it gets weird in this liminal space for sure. Have a look at the start/end events over in #1091 and see if that maybe makes a little more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Discussions
Development

Successfully merging this pull request may close these issues.

None yet

6 participants