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

chore(angular): Add Angular version to event contexts #5260

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jun 14, 2022

Query and send the Angular version in events to Sentry to let us know which Angular versions are used by our users. This should help us make data-based Angular version support decisions in the future.

Query and send the Angular version to Sentry to help us investigate which
Angular versions are used by our users to make support decisions
@Lms24 Lms24 requested a review from AbhiPrasad June 14, 2022 16:34
Comment on lines 42 to 45
const angularVersion = VERSION && VERSION.major ? parseInt(VERSION.major, 10) : 0;
const eventId = runOutsideAngular(() =>
Sentry.captureException(extractedError, { contexts: { angular: { version: angularVersion } } }),
);
Copy link
Member

Choose a reason for hiding this comment

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

Should we configure the scope globally on init to do this?

packages/angular/src/sdk.ts

export function init(options: BrowserOptions): void {
  options._metadata = options._metadata || {};
  options._metadata.sdk = {
    name: 'sentry.javascript.angular',
    packages: [
      {
        name: 'npm:@sentry/angular',
        version: SDK_VERSION,
      },
    ],
    version: SDK_VERSION,
  };
  const angularVersion = VERSION && VERSION.major ? parseInt(VERSION.major, 10) : undefined;
  if (angularVersion) {
    checkAngularVersion(version);
    Sentry.setContext('angular', { version: angularVersion });
  }
  browserInit(options);
}

Also regarding const angularVersion = VERSION && VERSION.major ? parseInt(VERSION.major, 10) : undefined;. Should we store the entire version? Or just the major like we do now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea, thanks! will change it!

Copy link
Member Author

Choose a reason for hiding this comment

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

About entire vs major: Not sure what's better for data querying. I think for the particular use case why we're doing this, the major should be enough but overall it might be worth collecting the full version (which begs the question if we should collect it in one string or in major, minor, patch properties). Do you know what's the better way of handling this (w.r.t indexing/querying in looker)?

Copy link
Member

Choose a reason for hiding this comment

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

Realized this didn't comment 😓

I think let's stick with the major for now since we get the most insights out of it. We can change this in the future if needed.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 14, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.43 KB (added)
@sentry/browser - ES5 CDN Bundle (minified) 60.21 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.24 KB (added)
@sentry/browser - ES6 CDN Bundle (minified) 53.83 KB (added)
@sentry/browser - Webpack (gzipped + minified) 20 KB (added)
@sentry/browser - Webpack (minified) 65.2 KB (added)
@sentry/react - Webpack (gzipped + minified) 20.02 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.02 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.63 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.17 KB (added)

@Lms24 Lms24 requested a review from AbhiPrasad June 15, 2022 09:30
@Lms24 Lms24 force-pushed the lms-angular-version branch 2 times, most recently from d930b3b to 829d3d2 Compare June 15, 2022 09:31
@Lms24 Lms24 self-assigned this Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants