DataDog / browser-sdk

Datadog Browser SDK
Apache License 2.0
294 stars 131 forks source link

RUM not automatically working with HashRouter #540

Open eduardogspereira opened 4 years ago

eduardogspereira commented 4 years ago

Hi!

I'm integrating the @datadog/browser-rum with at our react (16.12.0) application that uses react-router-dom (5.1.2) to manage the routers.

We are currently using HashRouter and due to already existing analysis data, we can't migrate from HashRouter to BrowserRouter without having the /# prefixed.

Basically, the issue we have is that the @datadog/browser-rum seems not to be integrating with HashRouter very well. I uploaded some code I was using to debug here https://github.com/eduardogspereira/datadog-debug, but basically I'm setting the datadogRum and router like this:

image

Even that the hashchange event is being triggered, the paths are not being correct updated at datadog:

image


Also, we trying to migrate from HashRouter to BrowserRouter keeping the /# prefixed. Like this:

image

But with this approach the datadogRum don't update the locationPath also.


It's possible that this is a bug? Thanks!

BenoitZugmeyer commented 4 years ago

Hello Eduardo! Thanks for your report.

In the Datadog UI, the "path" column is the actual URL path, and you should be able to add a column on @view.url_details.hash to see the hash.

I agree that in the "Hash router" case this is not convenient, having the hash directly would be better. I'll raise the issue internally.

eduardogspereira commented 4 years ago

oh, I see. I didn't notice it, thanks! I'll add the @view.url_details.hash column here.

Btw, would be awesome to see the @view.url_details.hash at this screen too :smile::

image

rbaxter08 commented 3 years ago

Btw, would be awesome to see the @view.url_details.hash at this screen too 😄:

image

I noticed this same issue today when looking into adding RUM to our app. Would be awesome to have the @view.url_details.hash displayed on that view!

BenoitZugmeyer commented 3 years ago

We are working on a solution for this, where the user can specify a view "name" to be displayed instead of the view path in the Datadog UI. You can have a sneak peek here: https://github.com/DataDog/browser-sdk/pull/724 . With this, it will be trivial to use the view hash instead of the path, with an API that may look like:

DD_RUM.init({
  // ...
  onNewLocation(newLocation) {
    return { viewName: newLocation.hash }
  }
})

Woud this looks like a good solution for you?

rbaxter08 commented 3 years ago

I think that's a pretty solid solution, seems nice and flexible for the users. Thanks for the link to the PR!

bcaudan commented 3 years ago

Hello!

Since browser-sdk@2.17.0, you can now track views manually and define your own view names. More info on the documentation.

strowk commented 3 years ago

Why this cannot work OOB? I think instead of having only path in View path, adding hash there would make sense as well, at least with opt-in in init call?

I think modifying source code, adding some hooks for router when route changes and calling datadog on that - would be prone to errors. Especially because this feature specifies No RUM data is collected until the view is started.. So in case if something is broken in the router itself - we will also loose data and will not see data from our (real) users, which kindof defeats the purpose of using RUM. Please add hash to the "View path" by default, or allow us to opt-on that behavior

bcaudan commented 3 years ago

Hi @strowk,

We could consider to introduce this kind of configuration at some point but it is not in our priorities right now.

I think you could use beforeSend API to update events view url., in order to transform /#foo in /foo.

Would that work for you?

strowk commented 3 years ago

Hi @bcaudan , thanks for quick answer!

Hm, I see this snippet in example

import { datadogRum } from '@datadog/browser-rum';

datadogRum.init({
    ...,
    beforeSend: (event) => {
        // remove email from view url
        event.view.url = event.view.url.replace(/email=[^&]*/, "email=REDACTED")
    },
    ...
});

Tried it like this


        beforeSend: event => {
            console.log('window.location.hash', window.location.hash);
            event.view.url = window.location.hash;
        },

have this in log

window.location.hash #myhash 

No effect on RUM webconsole, still shows just '/' for view..

bcaudan commented 3 years ago

FMU, your code will transform view url this way:

https://www.domain.com/#hash -> #hash

Since you still need a valid URL, it could work better with something like:

event.view.url = event.view.url.replace('#', '')

resulting in:

https://www.domain.com/#hash -> https://www.domain.com/hash

strowk commented 3 years ago

I tried replacing hash with slash - this worked, sort of, but does not seem to be very stable. Sometimes I still get something like SPA Route Change / or Load Page / followed by Load Page /hash , even though I am sure client was always loading /#hash, I checked that Load Page / view and I can see that url was with /#hash, so sdk somehow firstly reported view without applying that function and then reported another view after applying function... I think such partial solution would be more confusing. Even though it gives somewhat more insight, I do not want to introduce this hack into codebase, which potentially can be broken and has to be maintained.

I will wait for this issue to get proper resolution and full support of hash router (or rejection if you choose to do so), before proceeding. In the meantime I guess we just have to ignore "view" in RUM webconsole

vzakharov-rxnt commented 2 years ago

I think hash routing should work out of the box. Should be no need to implement the following approach:

event.view.url = event.view.url.replace('#', '')