feat: Sessions Health Tracking #2973

kamilogorek · 2020-10-14T09:42:36Z

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

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

packages/browser/src/sdk.ts

packages/browser/src/transports/fetch.ts

packages/core/src/basebackend.ts

packages/core/src/baseclient.ts

github-actions · 2020-10-14T11:59:30Z

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

💯

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

rhcarvalho · 2020-10-14T11:56:37Z

packages/hub/src/hub.ts

+    const session = scope.getSession();
+    if (session) {
+      session.close();
+      if (client && client.captureSession) {


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

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

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

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

rhcarvalho · 2020-10-14T12:45:13Z

packages/hub/src/session.ts

+  public release?: string;
+  public sid: string = uuid4();
+  public did?: string;
+  public timestamp: number = Date.now();


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

It's other way around. It's utcnow() if user doesn't provide it explicitly :) (it's the only if that omits not 😅)

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.

It doesn't, as python's __init__ is calling update which will set it immediately to utcnow()

packages/hub/src/session.ts

packages/tracing/src/browser/browsertracing.ts

packages/types/src/hub.ts

packages/types/src/request.ts

packages/types/src/transport.ts

packages/core/src/baseclient.ts

packages/tracing/src/browser/browsertracing.ts

dashed · 2020-10-14T21:10:50Z

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


Suggested change

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

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

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

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

rhcarvalho

🌟

kamilogorek requested review from rhcarvalho, HazAT and lobsterkatie October 14, 2020 09:42

rhcarvalho reviewed Oct 14, 2020

View reviewed changes

rhcarvalho suggested changes Oct 14, 2020

View reviewed changes

kamilogorek added 3 commits October 14, 2020 15:57

feat: Sessions Health Tracking

a73d3ef

Remove unused logger import

bb2968a

Code review patches

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

d82f916

kamilogorek force-pushed the sessions branch from d9627e6 to d82f916 Compare October 14, 2020 13:57

rhcarvalho reviewed Oct 14, 2020

View reviewed changes

packages/core/src/baseclient.ts Outdated Show resolved Hide resolved

rhcarvalho reviewed Oct 14, 2020

View reviewed changes

packages/tracing/src/browser/browsertracing.ts Outdated Show resolved Hide resolved

dashed reviewed Oct 14, 2020

View reviewed changes

Rework _processEvent and minor review updates

45e263f

rhcarvalho approved these changes Oct 15, 2020

View reviewed changes

Merge branch 'master' into sessions

db493a1

kamilogorek merged commit 4b2d249 into master Oct 15, 2020

kamilogorek deleted the sessions branch October 15, 2020 12:14

lobsterkatie mentioned this pull request Feb 25, 2021

meta: bump sentry dependencies to lates 6.1.0 version getsentry/sentry-electron#297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Sessions Health Tracking #2973

feat: Sessions Health Tracking #2973

kamilogorek commented Oct 14, 2020 •

edited

Loading

rhcarvalho left a comment

github-actions bot commented Oct 14, 2020 •

edited

Loading

rhcarvalho left a comment

rhcarvalho Oct 14, 2020

kamilogorek Oct 14, 2020

rhcarvalho Oct 14, 2020

kamilogorek Oct 15, 2020

rhcarvalho Oct 14, 2020

kamilogorek Oct 14, 2020

rhcarvalho Oct 14, 2020

kamilogorek Oct 15, 2020

dashed Oct 14, 2020

dashed Oct 14, 2020 •

edited

Loading

kamilogorek Oct 15, 2020

rhcarvalho left a comment

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

feat: Sessions Health Tracking #2973

feat: Sessions Health Tracking #2973

Conversation

kamilogorek commented Oct 14, 2020 • edited Loading

rhcarvalho left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2020 • edited Loading

size-limit report

rhcarvalho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashed Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhcarvalho left a comment

Choose a reason for hiding this comment

kamilogorek commented Oct 14, 2020 •

edited

Loading

github-actions bot commented Oct 14, 2020 •

edited

Loading

dashed Oct 14, 2020 •

edited

Loading