Skip to content
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

[EBT] Add the page's title and the URL's path to the events' context #149249

Closed
afharo opened this issue Jan 19, 2023 · 16 comments
Closed

[EBT] Add the page's title and the URL's path to the events' context #149249

afharo opened this issue Jan 19, 2023 · 16 comments
Assignees
Labels
enhancement New value added to drive a business result Feature:Telemetry Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@afharo
Copy link
Member

afharo commented Jan 19, 2023

Adding the title of the page and the URL's pathname (without the host and the query params but with the hash) would help us identify in which view the event was generated.

This is needed by @alexmarhaba

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc triage_needed enhancement New value added to drive a business result Feature:Telemetry labels Jan 19, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@alexmarhaba
Copy link

@afharo any chance we can get an ETA on this? It's going to be crucial for our information architecture revamp project.

@afharo
Copy link
Member Author

afharo commented Feb 6, 2023

@alexmarhaba, let me work out with the team the prioritization.

@afharo
Copy link
Member Author

afharo commented Feb 7, 2023

Some implementation notes:

Reacting to title changes:

it looks like we can react to title changes using the MutationObserver. Here's an example of how to achieve that: https://stackoverflow.com/a/29540461/17967885

We then need to register a context provider via analytics.registerContextProvider, where we emit a new value every time it changes.

Reacting to url changes:

We can listen to the popstate event. It triggers whenever the browser's history receives a new entry.

Here's a small snippet testing those 2 observers:

<html>
  <header>
    <title>Test</title>
  </header>
  <body>
    <a href="#" onclick="document.title = `Title ${Date.now()}`">Click here!</a>
    <script>
      // React to title changes
      const observer = new MutationObserver((change) => {
        console.info(`Title changed: ${change[0].target.textContent}`);
      });
      observer.observe(
        document.querySelector('title'),
        { childList: true }
      );

      // React to URL changes
      window.addEventListener('popstate', (ev) => {
        console.info(ev);
      })
    </script>
  </body>
</html>

@afharo
Copy link
Member Author

afharo commented Feb 7, 2023

notes from grooming:

  • probably worth looking at the react-router to provide generalized paths (like /app/dashboards/:id instead of /app/dashboard/{ACTUAL_DASHBOARD_ID}). This can be an additional property in case we cannot ensure all apps are using react-router.
  • Probably @elastic/appex-sharedux can take care of it since they'll likely be involved in that project? We can do this one together as sort of a training for further requirements 😇

@vadimkibana vadimkibana added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) and removed Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Feb 7, 2023
@vadimkibana vadimkibana self-assigned this Feb 7, 2023
@afharo
Copy link
Member Author

afharo commented Feb 8, 2023

I noticed #150357... it might help with the react-router item 😇

@rshen91
Copy link
Contributor

rshen91 commented Apr 10, 2023

In #150461 we have added the routeTitle to the announcement as the routeTitle. Could this help in terms of telemetry?

@afharo afharo added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) and removed Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels May 5, 2023
@shahinakmal
Copy link

just want to add a ++ on this if we can get this bumped in priority as it would add significant value for path reporting. thanks

@rshen91 rshen91 added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label May 16, 2023
@rshen91
Copy link
Contributor

rshen91 commented May 16, 2023

Hi @elastic/kibana-core I believe this issue might fall into your teams domains of telemetry? The page title is setup in packages/core/chrome/core-chrome-browser-internal/src/ui/header/screen_reader_a11y.tsx from the issue linked above and I spent some space time last week trying to figure out how to add that to the analytics client's context without much luck. If this is a high urgency issue, would your team be able to help? Thank you!

@pgayvallet
Copy link
Contributor

The page title is setup in packages/core/chrome/core-chrome-browser-internal/src/ui/header/screen_reader_a11y.tsx

No, I don't think it is. This component is adding a live region for the content of the Kibana breadcrumb, not for the title of the page (unless the breadcrumb is what you call the 'page title' here?).

I spent some space time last week trying to figure out how to add that to the analytics client's context

Have you tried the approach @afharo described in #149249 (comment)? It seems that using these to add the info to an analytic context provider should do the trick?

If this is a high urgency issue, would your team be able to help?

Probably, yes. Is it?

@afharo
Copy link
Member Author

afharo commented May 17, 2023

From a previous comment in this issue:

  • Probably @elastic/appex-sharedux can take care of it since they'll likely be involved in that project? We can do this one together as sort of a training for further requirements 😇

@vadimkibana agreed that the team would give it a go. Happy to help deliver this through.

@vadimkibana
Copy link
Contributor

Happy to help deliver this through.

Yes, please do.

I'll be un-assigning SharedUx from this, as Rachel gave it a go during her Space Time with no success. I'm not sure it is clear what needs to be done here and that the proposed methods of retrieving analytics insights are sound.

  • Listening for title changes through MutationObserver appears like a hack to me. I would expect that the title would be maintained by the Kibana ChromeService, and one could just subscribe to some observable.
  • Listening to popstate changes in DOM is not going to work as well, those events are broken, hence, we use the React Router with history package, one should subscribe to path changes in the history object in the ChromeService instead.

@afharo
Copy link
Member Author

afharo commented Jun 14, 2023

@vadimkibana, your comments make sense to me. ChromeService is owned by the SharedUX team. Does it mean, following the implementation you suggested, AppEx SharedUX will tackle this issue?

@pgayvallet
Copy link
Contributor

Listening for title changes through MutationObserver appears like a hack to me. I would expect that the title would be maintained by the Kibana ChromeService, and one could just subscribe to some observable.

Yeah, I think that's a fair assumption. In theory chome.setTitle is the only official way to update the page's title, so using the associated observer as the source of truth sounds reasonable to me.

Listening to popstate changes in DOM is not going to work as well, those events are broken, hence, we use the React Router with history package, one should subscribe to path changes in the history object in the ChromeService instead.

Yeah, creating an observable based from listening our global history seems the correct way to do this. Note that some applications are still using the hash-based router, therefor internal navigation from within those apps will not be detected with that approach, but it's still probably fine.

It probably means that the associated EBT context providers should be registered from within Core, given none of those two info are available externally atm (and that we likely don't want something like globalHistory.listen outside just for this)

@pgayvallet
Copy link
Contributor

FYI, I just opened #159936 and #159916 that cover each one of the requested context providers.

pgayvallet added a commit that referenced this issue Jun 20, 2023
## Summary

Part of #149249

Add a new EBT context providing the `page_url` field to events.

`page_url` is based on the current url's `pathname` and `hash`
exclusively (no domain, port, query param...)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
pgayvallet added a commit that referenced this issue Jun 20, 2023
## Summary

Part of #149249

Add a new EBT context providing the page_title field to events.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@pgayvallet
Copy link
Contributor

Both PRs have been merged, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Telemetry Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

No branches or pull requests

7 participants