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

feat: Sessions Health Tracking #2973

Merged
merged 5 commits into from Oct 15, 2020
Merged

feat: Sessions Health Tracking #2973

merged 5 commits into from Oct 15, 2020

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Oct 14, 2020 •
edited

Step 1:

Sentry.init({
  dsn: '__PUBLIC_DSN__',
  autoSessionTracking: true
});

Step 2: PROFIT

For now, the initial page load session is tracked using First-Contentful-Paint in conjunction with load event. If Performance API is not available, FCP will be skipped and resolved instantly, and only load event will be awaited on.
This is a good first step that we can iterate over if necessary.

rhcarvalho
rhcarvalho reviewed Oct 14, 2020
View reviewed changes
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Looking good. I appreciate the jsdoc strings and clear code that most of the time speaks for itself.

First pass of feedback since I see things are already changing :)

packages/browser/src/backend.ts Show resolved Hide resolved
packages/browser/src/sdk.ts Outdated Show resolved Hide resolved
packages/browser/src/sdk.ts Outdated Show resolved Hide resolved
packages/browser/src/transports/fetch.ts Outdated Show resolved Hide resolved
packages/browser/src/transports/fetch.ts Outdated Show resolved Hide resolved
packages/core/src/basebackend.ts Outdated Show resolved Hide resolved
packages/core/src/baseclient.ts Show resolved Hide resolved
packages/core/src/baseclient.ts Show resolved Hide resolved
packages/core/src/baseclient.ts Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2020 •
edited

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 19.59 KB (0%)
@sentry/browser - Webpack 20.42 KB (0%)
@sentry/react - Webpack 20.42 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 26.37 KB (0%)

rhcarvalho
rhcarvalho suggested changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

💯

Second pass. Up to you, I'd start with less attributes on Session unless we can clarify their usage.

What I care most is how we set the sent_at on the envelope header, that has to change.

packages/core/src/request.ts Outdated Show resolved Hide resolved
packages/hub/src/hub.ts
const session = scope.getSession();
if (session) {
session.close();
if (client && client.captureSession) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be a custom client that doesn't implement captureSession?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, why not? It's an optional method.

Copy link
Contributor

Choose a reason for hiding this comment

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

How optional? Aren't all clients based off BaseClient? The BaseClient abstract class implements captureSession.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our own yes, but not if you are implementing a custom one. We rely on Client interface, not BaseClient abstract class.

packages/hub/src/hub.ts Show resolved Hide resolved
packages/hub/src/session.ts
public release?: string;
public sid: string = uuid4();
public did?: string;
public timestamp: number = Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs are a bit ambiguous. I'd omit the parts of session we don't need for now.

In particular, the timestamp should be the time when "session change event came in". In Python it is set to datetime.utcnow() in Session.update, otherwise it is None.

https://github.com/getsentry/sentry-python/blob/8bd8044de7107c20b5318462142becb5b75c6315/sentry_sdk/sessions.py#L195-L196

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's other way around. It's utcnow() if user doesn't provide it explicitly :) (it's the only if that omits not 😅)

Copy link
Contributor

Choose a reason for hiding this comment

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

The if is what I linked above, and it in in the Session.update method (Python). Here (JS) we're looking at the timestamp at construction time, in Python it starts as None.

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 doesn't, as python's __init__ is calling update which will set it immediately to utcnow()

packages/hub/src/session.ts Outdated Show resolved Hide resolved
packages/tracing/src/browser/browsertracing.ts Outdated Show resolved Hide resolved
packages/types/src/hub.ts Outdated Show resolved Hide resolved
packages/types/src/hub.ts Show resolved Hide resolved
packages/types/src/request.ts Outdated Show resolved Hide resolved
packages/types/src/transport.ts Show resolved Hide resolved
dashed
dashed reviewed Oct 14, 2020
View reviewed changes
packages/browser/src/sdk.ts
try {
const po = new PerformanceObserver((entryList, po) => {
entryList.getEntries().forEach(entry => {
if (entry.name === 'first-contentful-paint' && entry.startTime < firstHiddenTime) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (entry.name === 'first-contentful-paint' && entry.startTime < firstHiddenTime) {
if (entry.name === 'first-contentful-paint' && entry.startTime < firstHiddenTime && !fcpResolved) {

Copy link
Member

@dashed dashed Oct 14, 2020 •
edited

Choose a reason for hiding this comment

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

Should guarantee that the if condition is run at most once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Observer is disconnected after the first call, so I'm not sure how it'd be ever called twice?

rhcarvalho
rhcarvalho approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

🌟

@kamilogorek kamilogorek merged commit 4b2d249 into master Oct 15, 2020
@kamilogorek kamilogorek deleted the sessions branch October 15, 2020 12:14
@lobsterkatie lobsterkatie mentioned this pull request Feb 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashed dashed dashed left review comments

@rhcarvalho rhcarvalho rhcarvalho approved these changes

@HazAT HazAT Awaiting requested review from HazAT

@lobsterkatie lobsterkatie Awaiting requested review from lobsterkatie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kamilogorek @rhcarvalho @dashed