-
Notifications
You must be signed in to change notification settings - Fork 278
feat: Add Browser click event #2992
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
base: main
Are you sure you want to change the base?
feat: Add Browser click event #2992
Conversation
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.
This seems too generic and instead I feel we should aim for tailored click, key etc events. Ideally at a mimum following https://www.w3schools.com/jsref/obj_events.asp but also potentially splitting a couple ie mouse & click. That way we can easily tailor attributes based on the trigger.
Also for click events Why not extend https://opentelemetry.io/docs/specs/semconv/app/app/#event-appwidgetclick but keep the same name?
| - id: browser.page.x | ||
| type: int | ||
| stability: development | ||
| brief: 'Click x (horizontal) coordinates (in pixels) relative to the entire document.' | ||
| examples: [10] | ||
| - id: browser.page.y | ||
| type: int | ||
| stability: development | ||
| brief: 'Click y (vertical) coordinates (in pixels) relative to the entire document.' | ||
| examples: [10] |
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.
Have we considered using https://opentelemetry.io/docs/specs/semconv/registry/attributes/app/#app-screen-coordinate-x etc
| - id: browser.tag_name | ||
| type: string | ||
| stability: development | ||
| brief: 'Target element tag name obtained via event.target.tagName.' | ||
| examples: ['BUTTON'] |
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 having multiple events makes sense. It would make it easier to know which attributes to expect vs a bunch of mixed possible attributes depending on type
I wasn't part of the original discussion but it seems they preferred to use browser terminology instead of translating e.g. Some of these
Component is a loaded term in web, it can be confused for example with a React component that can be composed of multiple dom elements.
IMO I would like to have shared events between client SDKs (web and mobile) but they work for both cases, |
So I have seen at https://www.w3schools.com/jsref/obj_mouseevent.asp and page/app are in effect different measurements so perhaps we go with app.page.coordinate.x.
I agree but I think we can improve the description to resolve those issues and use the attribute notes to define how to source it. For widget id = html element id and fall back to xpath when not defined. |
|
Could someone from the browser sig or a maintainer add the triage accepted label so that the hw.mouse.button can be added to hardware namespace and not have this PR auto closed? |
| - id: hw.mouse.button | ||
| type: | ||
| members: | ||
| - id: left | ||
| value: 'left' | ||
| brief: Left button | ||
| stability: development | ||
| - id: middle | ||
| value: 'middle' | ||
| brief: Middle button | ||
| stability: development | ||
| - id: right | ||
| value: 'right' | ||
| brief: Right button | ||
| stability: development | ||
| stability: development | ||
| brief: 'User friendly name of the mouse button pressed. See [MouseEvent.buttons](https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons#value).' | ||
| examples: ["left"] |
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.
These should be in the registry file in the hardware folder. But only make the change once the triage:accepted label has been added.
Changes
Added a new event for browsers named user_action.click. This event captures the "click" action in browsers
Prototype instrumentation:
open-telemetry/opentelemetry-browser#35
Important
Pull requests acceptance are subject to the triage process as described in Issue and PR Triage Management.
PRs that do not follow the guidance above, may be automatically rejected and closed.
Merge requirement checklist
[chore]