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

Session start and end events #1091

Merged

Conversation

breedx-splk
Copy link
Contributor

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

Changes

This serves as an alternative/successor to #1042. Instead of describing a session change event, we have a session start and session end event instead.

Merge requirement checklist

@breedx-splk breedx-splk requested review from a team May 29, 2024 16:52
@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.

LGTM. One non-blocking question, and a suggestion to mention how the previous session ID can be used to signify that the previous session has ended - but again, non-blocking

docs/general/session.md Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
@breedx-splk breedx-splk force-pushed the session_start_and_end_events branch from 2837df6 to 1ce116d Compare June 3, 2024 21:24
docs/general/session.md Outdated Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
@joaopgrassi
Copy link
Member

@breedx-splk I think it would be good to have folks from the event and client working groups to take a look and approve this? Are you in touch with them? We don't seem to have a GitHub team for them so not sure how to get attention.

Copy link
Contributor

@scheler scheler left a comment

Choose a reason for hiding this comment

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

Mostly looks good, left a couple comments. I envision having named sessions in the long term, but it can be added in the future using a session.name field.

docs/general/session.md Show resolved Hide resolved
docs/general/session.md Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
docs/general/session.md Outdated Show resolved Hide resolved
@breedx-splk
Copy link
Contributor Author

3 approvals, are we waiting for anything else on this? 🙏🏻

@lmolkova
Copy link
Contributor

@breedx-splk please resolve open discussions (if they are resolved) and it should be ready to go

@lmolkova lmolkova merged commit 93cc98b into open-telemetry:main Aug 1, 2024
13 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