feat(web-sdk): Properly allow users to override stack frame parsing settings

[elee1766]

Why

Certain extensions and libraries may throw exceptions with very long line lengths. These lines can cause the stack trace parser to lock up the browser thread, which is undesirable.

It would be desirable for sites that require the use of these extensions or libraries to be able to override stack trace parsing settings, such that they may sacrifice their stack trace quality for site performance.

What

1. the stackFrames package has been moved to web-sdk/src/utils. this is a breaking change, but consumers of the existing functions if they are importing from the top level package will not have to update their imports.

I did this because it currently lives under the error instrumentation package, but that doesn't make sense, as it is not only use by the error instrumentation package, but also the stack trace parser that is passed in the makeCoreConfig that ultimately gets used by the core config. it seems more correct for it therefore to live in utils.

2. i've added the ability to pass in ones own stack trace parser to the browser config. this will be respected and this function is what will passed as parseStacktrace in the faro config.

3. create function newStackTraceParser which creates a StacktraceParser using new type StackframeParserOptions, which includes a single optional field for maximumLineLength

4. make ErrorInstrumentation use the stacktraceParser from the faro global config so all this stuff is configured from one place

Links

#844

Checklist

• Tests added
• Changelog updated
• Documentation updated

[CLAassistant]

All committers have signed the CLA.

[elee1766]

should i write tests for the error instrumentation and passing in the parameters to the browser config?

[codecapitano]

Hi @elee1766 thanks a lot for the PR.
It's actually a good idea I think.
I only scanned over the PR. It may take some days till we have time to review it in depth.

Cheers,
Marco

[elee1766]

ok ive done this. i think this is a lot cleaner

[codecapitano]

Hey @elee1766 thanks a lot for all the changes.
I whent through the Faro code again and found out that it already has a changeStacktraceParser(...) function we can use to inject custom parsers. I totally forgot about that capability.

We need to leverage this functionality.
It's handled in the errors API module.
It provides a function to change the parser and also to get the parser.

Though ist's not respected by the auto instrumentations yet.
We can use the get getStacktraceParser() function which is provided by the error api to retrieve the configured stacktrace parser.
And then e. g. inside the registerOnunhandledrejection(...) function we can get the configured parser. `

Example

export function registerOnunhandledrejection(api: API): void {
  window.addEventListener('unhandledrejection', (evt: ExtendedPromiseRejectionEvent) => {
   //...

    } else {
      [value, type, stackFrames] = getErrorDetails(error, api.getStacktraceParser());
    }
    
   //...
  });
}

Let me know if you need more information or any support.

Cheers

Edit:
Looks like there is a bug in the types for the Faro config.
It already contains the stackTraceParse property but it's omitted in the types for the final Faro config (BrowserConfig).

[elee1766]

yeah so i didnt use it because that function wasnt respected by auto instrumentation (and also wasnt exposed, i think?)

i can do this

[codecapitano]

yeah so i didnt use it because that function wasnt respected by auto instrumentation (and also wasnt exposed, i think?)

Yes that's 100% correct.

• The custom parser was only respected on manual api call but not by the auto instrumentation
• The type was excluded on the BrowserConfig so it wasn't available via the Faro config.

[elee1766]

@codecapitano

so you said

Looks like there is a bug in the types for the Faro config.

digging deeper - it seems that this parseStackTrace is the initial setting for the Exceptions apis value for stacktraceparser, and the exceptions api then stores its own stacktraceparser i think in the function object scope or whatever for modification.

maybe it is correct to allow passing this parameter in from the web configuration?

its a bit confusing that the property here that supposedly would be passed to other things only modifies the exceptions api, and then wont necessarily match because of changeStackTraceParser.

changeStacktraceParser is sort of weird to be honest existing in the first place. its not even exposed right now. does it make more sense to just.. remove changeStacktraceParser?

[codecapitano]

@codecapitano

so you said

Looks like there is a bug in the types for the Faro config.

digging deeper - it seems that this parseStackTrace is the initial setting for the Exceptions apis value for stacktraceparser, and the exceptions api then stores its own stacktraceparser i think in the function object scope or whatever for modification.

Yes that's a bit weird. When changing the parser it is applied to API calls only but not to the auto instrumentation.

maybe it is correct to allow passing this parameter in from the web configuration?

Yes this should be the goal. Provide a property in the config to defined a custom parser.

its a bit confusing that the property here that supposedly would be passed to other things only modifies the exceptions api, and then wont necessarily match because of changeStackTraceParser.

That's correct. It's imho a bug.

changeStacktraceParser is sort of weird to be honest existing in the first place.

You mean having a function AND a config option at the same time right?
Yes I am all in to consolidate this into a single property for example.
Tbh I don't have any use case in mind where users need to change the stacktrace parser dynamically during a session.
+1 from my side to get rid of the function.

its not even exposed right now. does it make more sense to just.. remove changeStacktraceParser?

That's what I meant with "bug in the Config type"

cheers

[codecapitano]

Maybe this helps a bit (just in case if didn't already find it).

• The default stacktrace parser is added to the config in makeCoreConfig
• Under the hood It utilises the same stackFrameParser as used in the auto instrumentation
• The stackTraceParser property isn't exposed because it's omitted in the BrowserConfig type. I can only imagine that back then this was done in favour of the changeStacktraceParser function.

[elee1766]

Tbh I don't have any use case in mind where users need to change the stacktrace parser dynamically during a session.
+1 from my side to get rid of the function.

ok. this makes sense to me. I think that dynamically changing the stacktrace parser might have been used to make testing simpler or something.

I will make these changes then and see how it looks. I like the idea of having a single source of truth configuration variable for the stack trace parser.

[codecapitano]

Tbh I don't have any use case in mind where users need to change the stacktrace parser dynamically during a session.
+1 from my side to get rid of the function.

ok. this makes sense to me. I think that dynamically changing the stacktrace parser might have been used to make testing simpler or something.

I will make these changes then and see how it looks. I like the idea of having a single source of truth configuration variable for the stack trace parser.

Great and many thanks 🙏

[elee1766]

hi @codecapitano sorry for the delay.

let me know how this looks.

[codecapitano]

hi @codecapitano sorry for the delay.

let me know how this looks.

@elee1766 many thanks ad also sorry from my side for the delay.
Not sure if I've time this week for the review and it'll likely be next week.

Cheers

[codecapitano]

Let's use parseStacktrace from the config? under the hood it's the same parsing logic.
We can update parseStacktrace to take the optional options object.

[codecapitano]

Another option could be to receive the parser from api.getStacktraceParser
Looks like we currently have three functions doing teh same thing.

[elee1766]

in actual usage the parser is received here by api.getStacktraceParser

[codecapitano]

Same here, why not use parseStacktrace?

[elee1766]

@codecapitano are you asking why not just use the function instead of the um, stacktracer parser interface?

yeah i dont know really. I'm happy to get rid of the stack trace parser interface and unify everything to just use the parseStackTrace function type.

i only did it like this because StacktraceParser interface existed so i thought there was some reason you guys wanted it to exist so i kept using it. not sure if there is some reason its there

[codecapitano]

@codecapitano are you asking why not just use the function instead of the um, stacktracer parser interface?

yeah i dont know really. I'm happy to get rid of the stack trace parser interface and unify everything to just use the parseStackTrace function type.

i only did it like this because StacktraceParser interface existed so i thought there was some reason you guys wanted it to exist so i kept using it. not sure if there is some reason its there

Yes it's a bit unfortunate but it's a great opportunity to clean things up here and make things a bit easier to maintain.

[elee1766]

ok so what i did here was remove getStacktraceParser.

i think i prefer this so that people can do fully custom implementation instead of having them pass options.

[elee1766]

hi @codecapitano bumping here