- 
                Notifications
    You must be signed in to change notification settings 
- Fork 278
Add Browser UserAction Event #1941
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 UserAction Event #1941
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.
I think this is good, especially as a first pass "development" event, thanks! I left some comments and think there is room for improvement and possibly sharing/broadening, but I don't want those to block. Thanks for making this bite-sized!
| - id: keyboard.enter | ||
| value: "keyboard.enter" | ||
| stability: development | ||
| brief: An element is entered via keyboard by a user. | 
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 description is not clear to me. is that similar to keyboard.space below? so we track when user hits enter while an element (probably an input) has the focus?
If so I wonder why we want to differentiate between these two keys.
| value: "keyboard.space" | ||
| stability: development | ||
| brief: A space is entered via keyboard by a user. | ||
| - id: other | 
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.
another interesting event IMHO is input
| - id: page.x | ||
| type: int | ||
| stability: development | ||
| brief: Click x(horizontal) coordinates(in pixels) relative to the entire document. | ||
| requirement_level: recommended | ||
| examples: | ||
| - 10 | ||
| - id: page.y | ||
| type: int | ||
| stability: development | ||
| brief: Click y(vertical) coordinates(in pixels) relative to the entire document. | ||
| requirement_level: recommended | ||
| 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.
Could we use the app.screen.coordinate.* namespace  ?
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.
#1941 (comment) and #2070 should already cover the app click events
| - id: tags | ||
| type: string[] | ||
| stability: development | ||
| brief: Grab data from data-otel-* attributes in tree. | ||
| requirement_level: recommended | ||
| examples: | ||
| - ["id", "name"] | 
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.
Attributes should be set as attributes on the event and don't think that these need to be defined here.
| - id: type | ||
| type: enum | ||
| members: | ||
| - id: click.left | ||
| value: "click.left" | ||
| stability: development | ||
| brief: An element is left clicked by a user. | ||
| - id: click.right | ||
| value: "click.right" | ||
| stability: development | ||
| brief: An element is right clicked by a user. | ||
| - id: click.middle | ||
| value: "click.middle" | ||
| stability: development | ||
| brief: An element is middle clicked by a user. | ||
| - id: scroll | ||
| value: "scroll" | ||
| stability: development | ||
| brief: An element is scrolled by a user. | ||
| - id: zoom | ||
| value: "zoom" | ||
| stability: development | ||
| brief: An element is zoomed by a user. | ||
| - id: resize | ||
| value: "resize" | ||
| stability: development | ||
| brief: An element is resized by a user. | ||
| - id: keyboard.enter | ||
| value: "keyboard.enter" | ||
| stability: development | ||
| brief: An element is entered via keyboard by a user. | ||
| - id: keyboard.space | ||
| value: "keyboard.space" | ||
| stability: development | ||
| brief: A space is entered via keyboard by a user. | ||
| - id: other | ||
| value: "other" | ||
| stability: development | ||
| brief: User actions that are not listed above. | ||
| stability: development | ||
| brief: > | ||
| Type of interaction. | ||
| See enum [here](https://github.com/microsoft/ApplicationInsights-JS/blob/main/extensions/applicationinsights-clickanalytics-js/src/Enums.ts) for potential values we could add support for. | ||
| requirement_level: required | ||
| examples: ["click.right"] | 
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.
Feel that these should be attributes.
| - id: tag_name | ||
| type: string | ||
| stability: development | ||
| brief: Target element tag name and it is obtained via `event.target.tagName`. | ||
| requirement_level: recommended | ||
| 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.
Feel that this should be an attribute
| brief: A unique ID representing this particular metric instance. | ||
| requirement_level: required | ||
| examples: ["v3-1677874579383-6381583661209"] | ||
| - id: event.browser.user_action | 
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 to see the id/name be related to the group they come from with the list defined at: https://www.w3schools.com/jsref/obj_events.asp that way we can have a more tailored body/attributes.
| This PR was marked stale due to lack of activity. It will be closed in 7 days. | 
| What's the status of this PR, is this ready for the general SemConv review? | 
| Since @Karlie-777 is not actively working on this I am continuing her work here #2992 (I couldn't push to the same branch unfortunately). I am already working on the instrumentation side and we want to keep things moving @thompson-tomo Could you take another look there? For starters I moved all the body fields to attributes, could you replicate any other comments you have here there so we can continue the conversation? @david-luna I see you also have a pending comment, I replaced the  @MSNev and @breedx-splk You both approved this PR, could you please take another look at the new PR? Thanks everyone! | 
| Closing this in favor of #2992 | 
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]