-
Notifications
You must be signed in to change notification settings - Fork 280
Add browser page view event #1910
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?
Add browser page view event #1910
Conversation
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Co-authored-by: Trask Stalnaker <[email protected]>
|
I have made a few changes based on feedback:
There are a couple of things that still need to be resolved
|
| - id: browser.page_view.type | ||
| type: | ||
| members: | ||
| - id: physical_page | ||
| value: page_load | ||
| stability: development | ||
| brief: > | ||
| Initial page load within the browser which will generally also | ||
| precede a PageNavigationTiming event. | ||
| - id: virtual_page | ||
| value: soft_navigation | ||
| stability: development | ||
| brief: > | ||
| This is for Single Page Applications (SPA) where the framework | ||
| provides the ability to perform client side only page | ||
| "navigation", the exact definition of what a virtual page | ||
| change is determined by the SPA and the framework it is using. | ||
| stability: development | ||
| brief: Type of navigation | ||
| examples: ["page_load", "soft_navigation"] |
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 the other comment, I feel browser.page.view_type or browser.document.view_type might be a better fit as it is describing the page being shown in the browser.
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 here
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 wouldn't go with browser.document.view_type, I think we should reserve that namespace for attributes that are coming from window.document so it's easier to understand where is that coming from. I agree that it makes sense for referrer to be browser.document.referrer
It looks like we're dealing with two namespaces here:
- page
- page_view
Maybe having a proper definition of what they are can help assigning the attributes. If I have to give it a go I would say:
- page_view: it represents the action of navigating to a certain URL, whether is a full load (cold start) or soft navigation (like in SPAs)
- page: it contains information of the current page that the user is seeing in the browser, such us URL, title, etc
Having said that I think here we're trying to describe the navigation type, so I agree that it should be in the page_view 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.
I agree with @joaquin-diaz, the intent is to describe the type of navigation. Page itself does not have a type, especially when the values are either "page_load" or "soft_navigation". Maybe rename to browser.navigation.type?
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 agree the page doesn't have a type hence suggestion was view_type but I do like your suggestion of browser.navigation.type as it is clear what it is describing.
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.
Agree, I also like browser.navigation.type
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.
Same. There are other nav types: reload and back_forward. What about:
load
soft
reload
back_forward
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.
@martinkuba did we want to go with the change to browser.navigation.type especially as it would also potentially allow the additional types
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 discussed this in the SIG meeting last week and decided to keep it like this. Navigation type already exists in the navigation API, so did not want to overload it.
We might want to add navigation.type attribute in the future, but it would have this other meaning (captured from navigation.activation.navigationType).
Changes
This adds a new event for capturing the page view event from browser/web applications.
Related to open-telemetry/opentelemetry-browser#13
Merge requirement checklist
[chore]