CORE-951 - Use beforeCapture to set level in Sentry scope

[TylerZeroMaster]

https://openstax.atlassian.net/browse/CORE-951

Only set level if there is a reason to (override exists)

beforeCapture (Function)
(Available in version 5.20.0 and above)

A function that gets called before an error is sent to Sentry, allowing for extra tags or context to be added to the error.

...and a helpful link to the source too.

[TomWoodward]

These changes look fine but looking at the file I'm not sure how calling setError actually gets reported to sentry, where does that happen?

[TomWoodward]

Nevermind, I see it's called here, we aren't calling setError externally we're calling that

ui-components/src/hooks.ts

Line 17 in be1777d

eventId: Sentry.captureException(error)

[TomWoodward]

ScoresSyncError is only defined within lti-gateway, we should make this solution extensible so that the implementing app can specify the error levels when it defines the error displays

[TylerZeroMaster]

@TomWoodward That the error is defined in assignments.

I considered an alternative approach of having the other packages attach additional metadata, such as level, to their errors and then referencing that metadata here. That would have prevented direct dependence on error names. I used this approach to be consistent. This is already handling SessionExpiredError in a similar manner in the fallback rendering. That error is defined in ts-utils.

If the other approach is preferable, then I wonder if I should try to change the fallback rendering too. Maybe that should be a different issue.

[TomWoodward]

i think to avoid a massive refactor of the dependencies, it might be best to pass a map of error names and log levels into the boundary as a separate prop

[TylerZeroMaster]

Ok, but I think that would still require refactoring each instance of ErrorBoundary that could encounter SessionExpiredError or ScoresSyncError. That is, unless we want to have a default, but then we have the same problem with assumptions about error TYPE remaining the same. If we added level to some errors, specifically only the ones we want reported as warnings, then we could use that level if it exists, or default to reporting them as errors. My approach would require less refactoring and have a more global effect, but yours would be more explicit and potentially easier to follow.

Maybe I am assuming too much. The issue makes it sound like these errors should always have level set to warning. It sounds like you may want more granular control.

[TylerZeroMaster]

Right now I have a separate prop for error levels map with a default (similar to default fallbacks). I still feel like this is kind of brittle since there's nothing that would tell us if the TYPE of ScoresSyncError changes. Personally, I would prefer to define a default error level on the error class, like TYPE, and use that. Then we could keep this error levels map as a way to override the level set on the error class or have a different level for some errors in particular one-off cases. This would allow for maximum granularity while also reducing the number of assumptions made in ui-components. Here's an example of how it might work:

// ... (assignments)
export class ScoresSyncError extends Error {
  static readonly TYPE = 'ScoresSyncError';
  static readonly DEFAULT_LEVEL = 'warning';
  failures: Failure[];
  totalGrades: number;

  constructor(failures: Failure[], totalGrades: number  ) {
    const uniqueMessages = uniq(failures.map((failure) => failure.message));
    super(uniqueMessages.length === 1 ? uniqueMessages[0] : `${uniqueMessages.length} errors encountered`);
    this.failures = failures;
    this.totalGrades = totalGrades;
  }
}
// ...

// ... (ui-components)
if (error) {
  const type = getTypeFromError(error);
  const defaultLevel = getDefaultLevelFromError(error);
  // If we find a level in the map, that takes priority over default
  const errorLevel = errorLevels[type];
  if (errorLevel) {
    scope.setLevel(errorLevel);
  } else if (defaultLevel) {
    scope.setLevel(defaultLevel);
  }
  // otherwise this would be an error.
}

That would mean that we would not need to reference the TYPE in ui-components, but we could still do it that way in special situations from the other packages. The only downside I can think of to this approach is that it might be confusing to have multiple ways to set error level.

[TomWoodward]

adding a LOG_LEVEL static to the error class is fine with me if thats how you would prefer to do it.

[TylerZeroMaster]

@TomWoodward I ran into some problems accessing static members. I read a bit about what causes these problems and that made me hesitant to add more static members like TYPE. It also prompted me to check my initial assumption that TYPE could be used to map to error level. Using TYPE to map back to an error level does seem to work as expected.

If the current approach--having a default map of error names and log levels with a prop that supports overriding the defaults--looks good to you, then I would rather do it this way and avoid adding more static members to the errors. It is a little bit brittle, but it's also more explicit, probably adds less to the cognitive load, and requires less refactoring.

[TomWoodward]

waiting for revisions

[TomWoodward]

i still don't want to reference ScoresSyncError in this repo because its not defined here. i think SessionExpired might be defined here, or else its so generic (defined in ts-utils maaybe?) that its used everywhere, but the scores sync error is only relevant in assignments i think

[TylerZeroMaster]

i still don't want to reference ScoresSyncError in this repo because its not defined here. i think SessionExpired might be defined here, or else its so generic (defined in ts-utils maaybe?) that its used everywhere, but the scores sync error is only relevant in assignments i think

OK, I can see your point there. I will make a PR for assignments as part of this issue to add the error levels mapping.

---

As for the problems I ran into with static member access:

I wanted to add an error for unit testing, so I did something like this:

// src/components/ErrorBoundary.spec.tsx

class WarningError extends Error {
  static readonly LOG_LEVEL = 'warning';

  constructor(readonly a: string, readonly b: string) { }
}

When it came time to test, however, I could not get LOG_LEVEL. I tried a variety of approaches including:

• (error.constructor as any).LOG_LEVEL
• Object.getPrototypeOf(error).LOG_LEVEL
• Object.getPrototypeOf(error).constructor.LOG_LEVEL

The problem was that error.constructor was always referencing Error, the base class, rather than WarningError and Error does not have the static members defined. After reading a few different sources (like Inheritance and the prototype chain and Object.prototype.constructor), I could not find an exact explanation about what is happening. I did read that this can happen if your derived error class does not have its own constructor, but mine does.

I even tried to copy/paste errorIsType from ts-utils and do something like this:

export const errorIsType = <T extends {TYPE: string}>(t: T) => (e: any): e is T =>
  e instanceof Error
  && (e as any).constructor.TYPE === t.TYPE;

// ...

export class WarningError extends Error {
  static readonly TYPE = 'WarningError';

  constructor(stuff: number, readonly otherStuff: string) {
    super(`${stuff}`);
  }
}

// ...

console.log(errorIsType(WarningError)(new WarningError(20, 'otherstuff'))); // false

I also tried adding an instance method that returns the static member:

export class WarningError extends global.Error {
  static readonly TYPE = 'WarningError';

  constructor(stuff: number, readonly otherStuff: string) {
    super(`${stuff}`);
  }

  getType() {
    return WarningError.TYPE;
  }
}

Then I tried a variety of approaches to getting the instance method and instance properties. I was able to get the instance property, otherStuff, but the instance method, getType was missing.

// I create and throw the error in the unit test like this:

    const error = new WarningError(20, 'otherstuff');

    const WarningErrorComponent = () => {
      throw error;
    };

// Then I log details of each error like this:

console.log(
  Reflect.get(error, 'otherStuff'),
  Reflect.get(error, 'getType'),
  errorIsType(WarningError)(error)
);

The result looks like:

otherstuff undefined false

I tried an integration test by using this version of ui-components in Assignments and throwing an error in the book selection. In that case the type of the error was correctly recognized.

My theories of why this happens are:

1. Tree shaking and/or code splitting
2. jsdom test environment
3. Typescript translation problems
4. Maybe jest does something with error prototypes

There was an issue filed with jest where jest was hijacking Object.prototype which was causing some people to have problems with instanceof checks not working as expected. Although, that issue seems to have been related to babel. Granted, this is the closest I have come to finding an explanation and the version of jest that ui-components uses would be affected by this issue. I will try upgrading to the latest version of jest. If that fixes the issue then we will at least have some answers. No, same result.

[TylerZeroMaster]

I moved the mapping for ScoresSyncError into Assignments in this PR.