-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: Add additional fields collected by the TraceKit package for stack traces #383
Conversation
packages/honeycomb-opentelemetry-web/src/global-errors-autoinstrumentation.ts
Show resolved
Hide resolved
packages/honeycomb-opentelemetry-web/src/global-errors-autoinstrumentation.ts
Show resolved
Hide resolved
packages/honeycomb-opentelemetry-web/test/global-errors-instrumentation.test.ts
Show resolved
Hide resolved
|
||
it('should return an object with structured stack trace information', () => { | ||
expect(instr._computeStackTrace(new Error('This is an error'))).toEqual({ | ||
'exception.structured_stacktrace.columns': expect.any(Array), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to have this as a follow up PR but it would be great to assert on an actual transformation of a stack track here instead of checking that this returns an array. We would have to mock the Error.stack
property and with a sample stack trace and then check that the results are the line numbers, column numbers and urls that we expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a weird quirk with TraceKit
, mocking Error.stack
resulted in some lines being missing for certain stacks. Mocking Error.originalStack
works perfectly though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I'm gonna approve this as is and we can tackle these tests separately. Can you leave a TODO comment here for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also do this through smoke tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the TODO now
Short description of the changes
Introduces the TraceKit package to the web distro. Adds additional fields with the help of the
TraceKit
package. These additional fields are prefixed withexception.structured_stacktrace
. Additional fields will be sent byGlobalErrorsInstrumentation
.How to verify that this has the expected result
I tested locally using the
hello-world-web
example provided by the web-distro. Made the example to throw a simple error. This was the result I received on Honeycomb:Big thanks to @pkanal for helping with this change!