-
Notifications
You must be signed in to change notification settings - Fork 210
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
Add Browser Exception Event #1942
base: main
Are you sure you want to change the base?
Add Browser Exception Event #1942
Conversation
A stacktrace as a string in the natural representation for the language runtime. | ||
The representation is to be determined and documented by each language SIG. | ||
requirement_level: recommended | ||
examples: ["Exception in thread main java.lang.RuntimeException: |
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.
need to reformat the examples to make the generated table look nicer
name: browser.exception | ||
brief: > | ||
This event describes an error or exception that occurs in a browser application. | ||
body: |
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 would like to see a comment here to clarify that this is temporary or that this will actually be an attribute. That way readers do not expect to find content in the actual body.
@@ -0,0 +1,54 @@ | |||
groups: | |||
- id: event.browser.exception |
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.
Similar to comments I left on #1941, I think other client instrumentation would benefit from also having these relaxed to be non-browser. Alternatives: rum.exception
, client.exception
, app.exception
, runtime.exception
.....
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.
Yeah, I think if we can defined a "standard" exception event would be a better approach (things have evolved quite a bit since we originally defined this).
Even if this also means that as part of a "standard" exception event it would define and use the top-level
exception.*
attributes, but this may also mean that we should define an exception "source" to make life a little easier when attempting to directly compare values
-- community thoughts?
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.
@Karlie-777 can you please review open-telemetry/opentelemetry-js-contrib#2751 which is an initial implementation of a browser "event" which is using the existing top-level 3 attributes.
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.
Would like this to be available for ios/android as well, but otherwise looks good to me.
type: event | ||
name: browser.exception | ||
brief: > | ||
This event describes an error or exception that occurs in a browser application. |
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.
Lets also "add" a top-level attribute url.full
(redacted) or include a cutdown url
within the data), as while having the "filename" is useful, we should also be including the "hosting" page as the source file (bundle) may throw at different locations and for different reasons when used from different pages / scenarios.
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.
can we reuse the general exception event? https://github.com/open-telemetry/semantic-conventions/blob/main/docs/exceptions/exceptions-logs.md
we could add code.*
attributes as opt-in on the general exception event
and if you want them to be "recommended" specifically for browser, we can probably figure out how to do that
do you think other browser-specific attributes would be added to this event? if that's the case, then may make sense to keep the "browser" event name, but would still be good to "extend" the existing general exception event, and just layer in the additional attributes
Yeah, this is essentially what I'm saying also as we have had "exceptions" for spans and logs like for ages now, if we want to define a "standard" exception "event" (basically just give the Logs exception and event_name) then that would work, as long as we can add some required additional browser specific fields (I want to avoid having the discussions about "which" exception event should we create). One issue (which I think already exists to some extent today for different languages), is that for "browsers" specifically, there is no common "standard" of the "stacktrace", so different browsers (including between versions of browser) "present" the same stack trace with different formatting which makes it extremely problematic for backends to process and group the same basic common failure (vs a browser / runtime specific failure (like IE not liking missing comma's for example)), as such it is common (at least for the Microsoft JS Sdk's) to pass back the "raw" as well as a "normalized" (parsed) stack to pull out the common fields of a stack into their components (filename, line, col, source bundle) as the JS runtime often has more context about what the current runtime is (Opera, Chromium, Firefox, iOS, specific versions, as well as some runtimes that "act" (provide) like browser electron, etc).
|
Fixes #
Changes
Please provide a brief description of the changes here.
Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist
[chore]