DataDog / browser-sdk

Datadog Browser SDK
Apache License 2.0
279 stars 130 forks source link

beforeSend Editing View Events' Context #1659

Open indeediansbrett opened 1 year ago

indeediansbrett commented 1 year ago

Am looking at the docs below regarding editing view context. Am curious what was the reason context for views was made read-only? We were looking to change context, but for views only and want to make sure there's no potential consistency issues. https://docs.datadoghq.com/real_user_monitoring/browser/modifying_data_and_context/?tab=npm

bcaudan commented 1 year ago

Hi @indeediansbrett,

In our model, we ensure to have "parent" event context transmitted to the context of their "children" events. So a value of a view context should be present on the context of an action happening on this view.

In order to follow this model, we ensure that the view context can't be modified in the beforeSend, otherwise this modification would not be transmitted to subsequent events happening on this view.

What do you want to achieve by only changing the context of the view?

indeediansbrett commented 1 year ago

Essentially we have a context field as a list. For actions, resources, errors, and long task events we want it to be a snapshot. For views we want it to have all items throughout the course of the view.

What seems to be functioning as a workaround was to add an extra context field with the extra information for views. In beforeSend, we remove it from non-view events. We're running on a proxy, so in there we add the extra information into the original field for views and delete the extra context field.

Related to this is that we're manually tracking views. It seems like there's no function like a stopView similar to startView. Is there a way to stop sending data to RUM, change the context, then start the next view? We're deleting the extra context information for route change views, but have to do that after startView or else it changes the old view. Then there's a brief period of time where the first few document versions of the route change will have that context, but have to wait several document versions for it to disappear.

bcaudan commented 1 year ago

What seems to be functioning as a workaround was to add an extra context field with the extra information for views. In beforeSend, we remove it from non-view events.

Yes, that seems to be the best way to do it with current capabilities.

Related to this is that we're manually tracking views. It seems like there's no function like a stopView similar to startView. Is there a way to stop sending data to RUM, change the context, then start the next view? We're deleting the extra context information for route change views, but have to do that after startView or else it changes the old view. Then there's a brief period of time where the first few document versions of the route change will have that context, but have to wait several document versions for it to disappear.

Indeed, that seems the better way to achieve that for now.

We may want to introduce a specific view context to ease this use case but nothing planned yet. We'll keep you posted here when we will have more info to share about that.

mariaines commented 1 year ago

Hi, any update on this? I'm also wanting the ability to modify a view's context. I've tried implementing a workaround using global context as discussed above, but this doesn't work sometimes, I'm guessing due to conflicts when multiple events are being sent at the same.

In case helpful, my use case is that we massage the view URL and view referrer URL to handle some of our app's architecture-specific routing ~weirdness~ implementation, but we also want to be able to see the original, un-massaged URL. Specifically, we make some changes like:

29decibel commented 8 months ago

We have the similar issue here as well. What we want to achieve here is to attach the system owner of that event (error or view) according to the view URL. This seems very to be a very innocent requirement. We tried to use setGlobalContextProperty to get around this, but it's always a hit or miss. If this is a hard-no, then is there anyway we can attach more attributes in datadog after the event were collected? Thanks!

BenoitZugmeyer commented 8 months ago

@29decibel you could use trackViewsManually (see doc) for that. Because you are know in charge of creating views, you can attach global context reliably after the view is started:

DD_RUM.startView()
DD_RUM.setGlobalContextProperty({ system_owner: 'foo' })

Would that work for you?

29decibel commented 8 months ago

Hi @BenoitZugmeyer thanks for following up. This is what we were trying to do. But unfortunately it doesn't work 100%, as the collection from global context is a black box to us, we don't know when it's being collected. And we only have the startView API to call. What ended up happening to us is that if there are long xhr requests happening in the view1, then we saw it's being collected as view2.

DD_RUM.startView('view1')
DD_RUM.setGlobalContextProperty({ system_owner: 'foo' })

// some xhr is still not finished, or resource is still loading

// user start view2
DD_RUM.startView('view2')
DD_RUM.setGlobalContextProperty({ system_owner: 'bar' }) 

In this case some of the xhr requests will be classified as view2, because the setGlobalContextProperty globally changed the context.

That's why we are seeing in the dashboard that most of the requests are correctly tagged, but there are always quite some are not. Do you have a solution to this?

Also since we are on this context, maybe worth another ticket, but we also seeing that xhr requests are not collected at all in the view level in Datadog when using startView. But it works fine locally (localhost), do you happen to know any cause to that as well?

Thanks!

rorik commented 1 month ago

Hi @BenoitZugmeyer, I can provide one additional example as to why DD_RUM.setGlobalContextProperty() is not enough to add context to views:

This is where views are manually started:

/* Called on change of location.pathname or router params */
function onRouteCange(pathname: string, params: Record<string, string>) {
    const normalizedViewName = normalizeViewName(pathname, params);
    // The above function generalizes the view depending on the params: /a/123/b/c => /a/:id/b/:tab
    datadogRum.startView(normalizedViewName);
    datadogRum.setGlobalContextProperty("params", params);
}
Now looking at how the views are recorded in DataDog there's a problem: DATE VIEW NAME @CONTEXT.PARAMS
00:00:17 /zzz/:id {"id": "234"}
00:00:16 /zzz/:id {"id": "123"}
00:00:16 /yyy/d
00:00:13 /yyy/c {"id": "234"}
00:00:12 /yyy/b {"id": "123"}
00:00:08 /yyy/a
00:00:07 /yyy {"name": "cba"}
00:00:04 /xyz/:name {"name": "abc"}
00:00:03 /xyz/:name {"name": "abc"}
00:00:03 /xyz {"name": "abc"}
00:00:00 /

The above is a simplified version of a real session that I just recorded using the above onRouteCange method.

There's multiple issues:

I've validated that the startView and setGlobalContextProperty are called with the right values at the right time, so it's definitely an issue with how the sdk handles views/context. I've also tried changing the order of this functions, to set the context before starting the view, but that seems to be even worse.

Note that it works mostly fine in most of the views, but around 30-40% of the views recorded have the wrong context.

Seeing as how this system cannot be relied on, the only workaround we've found is to not use this context at all. Just create custom actions with the name of the view and add the params as context, that's guaranteed to be in sync.

Ideally when we call startView we can specify a context for it (either at startView or beforeSend). If all child events are required to have the same context as the view, they can inherit it without needing a global context. Something like:

datadogRum.startView("/zzz/:id/:tab", {id: "123", tab: "home"});
// ...
datadogRum.addAction("xyz", {z: "f"}); // the resulting context will be {id: "123", tab: "home", z: "f"}
// ...
datadogRum.startView("/xyz/:name", {name: "abc"});
// ...
datadogRum.addAction("xyz"); // the resulting context will be {name: "abc"}

I do please ask that the team considers implementing this functionality, it's a major blocker and supported by most of the other RUM tools in the market.

Thank you!

salazarm commented 1 month ago

Hi @BenoitZugmeyer,

I'm also experiencing a similar issue. In my case, I can't determine the view name when the view starts because it's defined by our custom Route component and only known when the route is rendered. Therefore, I need to set it asynchronously using beforeEvent. However, because the global context becomes intermixed (as described by rorik above), I can't use it to associate the view with the correct view name.

To work around this I give the view a name corresponding to a unique ID that is the key in an object that will receive the correct view name once its known. in beforeEvent I map the event.view.id to this uniqueID the first time I see it and then for all events of that view I use that mapping to retrieve the real view name.

salazarm commented 1 month ago

This is what my work around looks like:

const statesById = {};

let viewIdx = 0;
function onPushState() {
  const id = `view:${viewIdx++}`;
  statesById[id] = {viewName: '/' }; // this will be updated by dynamically rendered <Route> components
  datadogRum.startView({viewName: id})
}
const datadogUUIDToViewID: Record<string, string> = {};
...
beforeEvent: (event) => {
  if (datadogUUIDToViewID[event.view.id] === undefined) {
    datadogUUIDToViewID[event.view.id] = event.view.name!;
  }
  const id = datadogUUIDToViewID[event.view.id];
  if (id !== undefined) {
    const state = getState(id);
    if (state) {
      state.datadogViewId = event.view.id;
      event.view.name = state.viewName;
    } else {
      event.view.name = event.view.name + ' :UNKNOWN:' + id;
    }
  }
}
BenoitZugmeyer commented 4 weeks ago

Thank you for sharing your use-case. This is a frequently asked feature, I'll bring it again to the team.