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

[v7] Deprecated Enums Removal #4888

Closed
AbhiPrasad opened this issue Apr 7, 2022 · 3 comments
Closed

[v7] Deprecated Enums Removal #4888

AbhiPrasad opened this issue Apr 7, 2022 · 3 comments

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Apr 7, 2022

Problem Statement

Last December, we made some efforts to deprecate enums as they add excess bundle size. We plan to remove these in v7.

Here is a list of them, alongside things we must consider.

Solution Brainstorm

Will be deleted

Status

b177690

export type EventStatus =
  /** The status could not be determined. */
  | 'unknown'
  /** The event was skipped due to configuration or callbacks. */
  | 'skipped'
  /** The event was sent to Sentry successfully. */
  | 'rate_limit'
  /** The client is currently rate limited and will try again later. */
  | 'invalid'
  /** The event could not be processed. */
  | 'failed'
  /** A server-side error occurred during submission. */
  | 'success';

This is an internally used enum, so we can delete this without much repercussions.

SessionStatus

https://github.com/getsentry/sentry-javascript/blob/master/packages/types/src/sessionstatus.ts

export enum SessionStatus {
  /** JSDoc */
  Ok = 'ok',
  /** JSDoc */
  Exited = 'exited',
  /** JSDoc */
  Crashed = 'crashed',
  /** JSDoc */
  Abnormal = 'abnormal',
}

An internal enum, we can delete!

RequestSessionStatus

export enum RequestSessionStatus {
/** JSDoc /
Ok = 'ok',
/
* JSDoc /
Errored = 'errored',
/
* JSDoc */
Crashed = 'crashed',
}

An internal enum, we can delete!

Up for debate

Severity

export enum Severity {
  /** JSDoc */
  Fatal = 'fatal',
  /** JSDoc */
  Error = 'error',
  /** JSDoc */
  Warning = 'warning',
  /** JSDoc */
  Log = 'log',
  /** JSDoc */
  Info = 'info',
  /** JSDoc */
  Debug = 'debug',
  /** JSDoc */
  Critical = 'critical',
}

The Severity enum was removed in dd3aa70, but was later undeprecated in #4412.

The Severity enum is part of the public API - captureMessage call, so possibly would require a lot of user facing changes (they have to go in and change a bunch of methods). Do we decide to still move on with removing the enum?

public captureMessage(message: string, level?: Severity, hint?: EventHint): string {

SpanStatus

https://github.com/getsentry/sentry-javascript/blob/master/packages/tracing/src/spanstatus.ts

export enum SpanStatus {
  /** The operation completed successfully. */
  Ok = 'ok',
  /** Deadline expired before operation could complete. */
  DeadlineExceeded = 'deadline_exceeded',
  /** 401 Unauthorized (actually does mean unauthenticated according to RFC 7235) */
  Unauthenticated = 'unauthenticated',
  /** 403 Forbidden */
  PermissionDenied = 'permission_denied',
  /** 404 Not Found. Some requested entity (file or directory) was not found. */
  NotFound = 'not_found',
  /** 429 Too Many Requests */
  ResourceExhausted = 'resource_exhausted',
  /** Client specified an invalid argument. 4xx. */
  InvalidArgument = 'invalid_argument',
  /** 501 Not Implemented */
  Unimplemented = 'unimplemented',
  /** 503 Service Unavailable */
  Unavailable = 'unavailable',
  /** Other/generic 5xx. */
  InternalError = 'internal_error',
  /** Unknown. Any non-standard HTTP status code. */
  UnknownError = 'unknown_error',
  /** The operation was cancelled (typically by the user). */
  Cancelled = 'cancelled',
  /** Already exists (409) */
  AlreadyExists = 'already_exists',
  /** Operation was rejected because the system is not in a state required for the operation's */
  FailedPrecondition = 'failed_precondition',
  /** The operation was aborted, typically due to a concurrency issue. */
  Aborted = 'aborted',
  /** Operation was attempted past the valid range. */
  OutOfRange = 'out_of_range',
  /** Unrecoverable data loss or corruption */
  DataLoss = 'data_loss',
}

SpanStatus is another that has big savings if deleted, but provides a high amount of UX because it allows user's to see what exactly to set status to. We use it all around the docs (see the first example in https://docs.sentry.io/platforms/javascript/performance/instrumentation/custom-instrumentation/). Should we delete this?

@lforst
Copy link
Member

lforst commented Apr 8, 2022

Id make the decision based on a few factors:

  • How big is the effort for the user to adopt? (probably a lot bigger for Severity that for SpanStatus)
  • How big are the actual bundle savings?

Based on a gut feeling, I would not delete either of Severity and SpanStatus. I believe the effect of removing Severity would be too big with it being part of the captureMessage API. And SpanStatus, as you already mentioned, is quite valuable DX wise.


Additional Brainstom:

We might be able to save a tiny bit of bundle size save if we convert the enums into a plain objects.

@smeubank
Copy link
Member

if it is making the decision on a few - i would proceed with the ones where there is no doubt and we parking lot the others. If we have time during V7 betas we can make a decision then. Then the big decider for me is @lforst 2nd point how much bundle savings are there really to be had vs how much we could go ahead and ship to folks

@AbhiPrasad
Copy link
Member Author

Closing as a decision has been made. We are not deprecating Severity and SpanStatus, but we are deprecating the rest of the enums.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants