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.previous_id #348

Merged

Conversation

breedx-splk
Copy link
Contributor

@breedx-splk breedx-splk commented Sep 26, 2023

Changes

Introduces a convention to represent the previous session id for client instrumentation. This can be used by telemetry backends to create links from the old/expired session to the new one.

Existing art exists in Android via the SessionIdChangeTracer.

Merge requirement checklist

@breedx-splk breedx-splk requested review from a team September 26, 2023 21:35
@AlexanderWert
Copy link
Member

I think I get the idea of this proposal. But I think it would help if we had mobile- or client-specific semantic conventions that would describe how these attributes are intended to be used. I think it's clear for session.id (i.e. it would be just an attribute that's being added to all mobile/client signals/events, except for metrics due to cardinality).

But, what about session.previous_id? Would that be set on every signal/event as well (e.g. every mobile span would always have session.id and session.previous_id)? Or, is the intention rather to send a specific event that would indicate a "session transition" from old session to new session that would contain those two attributes?

I think documenting this reasoning as mobile semantic conventions would be great!

@breedx-splk
Copy link
Contributor Author

But, what about session.previous_id? Would that be set on every signal/event as well (e.g. every mobile span would always have session.id and session.previous_id)? Or, is the intention rather to send a specific event that would indicate a "session transition" from old session to new session that would contain those two attributes?

Yeah, I linked to the tracer in android to demonstrate, but yeah, the idea is to send a session change event. I can add some text in this same file to give more of an explanation of intent. Is this the right place for that, or, because it's behavioral does it belong in the spec repo?

@AlexanderWert
Copy link
Member

Yeah, I linked to the tracer in android to demonstrate, but yeah, the idea is to send a session change event. I can add some text in this same file to give more of an explanation of intent. Is this the right place for that, or, because it's behavioral does it belong in the spec repo?

I think a new area of semantic conventions under /docs/mobile/ would make sense (in addition to this PR, where just the session is being defined). In /docs/mobile/ we then can describe all the mobile-specific conventions such as types of mobile spans and their attributes to capture, description of mobile events to send, etc.

With #197 the definition of the session attributes would go into the registry anyways and the mobile semantic conventions would just reference it.

@breedx-splk breedx-splk requested review from a team October 3, 2023 21:59
@breedx-splk breedx-splk force-pushed the add_previous_session_id branch from 34bca22 to 08dcc03 Compare October 3, 2023 21:59
@breedx-splk breedx-splk force-pushed the add_previous_session_id branch 2 times, most recently from 8f485a5 to e0f9096 Compare October 12, 2023 21:12
@AlexanderWert
Copy link
Member

@open-telemetry/semconv-mobile-approvers Can we get more reviews on this one please?

@breedx-splk breedx-splk force-pushed the add_previous_session_id branch from 1676bde to 0fbb0ed Compare October 18, 2023 21:11
@breedx-splk
Copy link
Contributor Author

Hi @open-telemetry/specs-semconv-maintainers -- we have 4 approvals on this. Any reason to hold this up? 🙏🏻

@AlexanderWert AlexanderWert merged commit 889b0a4 into open-telemetry:main Oct 21, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants