Ignore errors

[adebayor123]

Details

Previously, CW RUM did not provide an option to allow customers to filter out error events that provided no monitoring value.
This PR addresses the issue by allowing the customers to pass in a function that acts similar to Array.filter to indicate whether the error should be recorded or not.

Resolves #119

Testing

Added a new unit test and an integ test to cover the new behavior.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[AntiMoron]

Great job

[maniator]

Why are we passing this default here?

Shouldn't this be in the plugin itself?

[adebayor123]

Not sure what you mean here - can you clarify a bit more?

[maniator]

Where is this file used?

is it a test/fixture file?

if so, should be noted in the filename

[adebayor123]

Loader files are used to load the web client in our test applications - we have different loader files for different test applications, as it helps us isolate the functionalities and behaviors we want to test.

I think the naming convention we have indicates that decently

[maniator]

Isn't 'shouldIgnoreError' optional?

[adebayor123]

So I have to admit, I had a wrong understanding of PartialConfigs and default values.. Now it makes sense that (1) default values should exist for non-optional parameters or else the code will break, (2) if a parameter is optional, the code should not rely on it.

[maniator]

Also doing an early exit instead of wrapping in an if statement may be better here

[adebayor123]

are you suggesting something in the lines of this?:

private promiseRejectEventHandler = (event: PromiseRejectionEvent) => {
        if (this.config.filter(event)) {
            return;
        }
        this.recordJsErrorEvent({
            type: event.type,
            error: event.reason
        } as ErrorEvent);
    };

[maniator]

Also should have a test to make sure it is called a certain amount of times with a mix of errors

[qhanam]

A few things here:

1. Use verbs or verb phrases for method names. I recommend filter, as developers will be familiar with the behavior of the Array.filter function in the JavaScript SDK.
2. We provide a default implementation below (() => false), which is a good pattern, so we can omit the ? here.
3. We'll need to support both ErrorEvent and PromiseRejectionEvent.

Suggested change

	shouldIgnoreError?: (error: ErrorEvent) => boolean;
	filter: (error: ErrorEvent | PromiseRejectionEvent) => boolean;

[qhanam]

From the MDN docs on the filter function's callback:

callbackFn
Function is a predicate, to test each element of the array. Return a value that coerces to true to keep the element, or to false otherwise.

In our case, the filter function will be the callback function, but I think the intent will be clear.

[qhanam]

issue: I think we want to add the filter to promiseRejectEventHandler separately, so that the filter will get the complete event in both cases. We'll also need to avoid calling eventHandler from promiseRejectEventHandler so the filter doesn't get applied twice (or something to that effect).

[qhanam]

question: For record, we don't want the error to pass through the filter at all right? The application has explicitly asked us to record it.

I suggest doing an extract method refactor on eventHandler so that the three functions still share common code, but, the original eventHandler only does the filter check.

[adebayor123]

Made the following changes:

1. Pull out the actual event creating activity into recordJsErrorEvent(error: any) method. This method simply records the error event, and is used across record(), promiseRejectEventHandler(), eventHandler().
2. record() will directly invoke recordJsErrorEvent instead of going through eventHandler to avoid going through filtering logic.
3. promiseRejectEventHandler will now do its own filtering and call recordJsErrorEvent if the error should be recorded