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

1ms delay before fetch causing issues with New Relic SPA logger #345

Closed
smblee opened this issue May 14, 2021 · 8 comments · Fixed by #351
Closed

1ms delay before fetch causing issues with New Relic SPA logger #345

smblee opened this issue May 14, 2021 · 8 comments · Fixed by #351

Comments

@smblee
Copy link

smblee commented May 14, 2021

I found a finicky bug where the implementation detail of ky waiting 1ms before fetch (#118) is actually not allowing New Relic Pro+SPA to log AJAX requests.

I am not sure of the full detail of why this may be happening - but this is how i have verified it's being caused by 1ms setTimeout.

  • I tried using native fetch and wretch library instead of ky. New Relic was able to pick up all AJAX requests for both scenarios.
  • I tried using axios that relies on XHR requests. New Relic was also able to pick this up.
  • I used ky but manually commented out the await delay(1); from ky inside node_modules, and it started working (granted, the .json function fails, but the request was getting picked up)

Do you guys have any idea why this may be happening?

If so, do you have suggestions on how to get around this issue?

@smblee
Copy link
Author

smblee commented May 14, 2021

#220 linking this issue for more exposure

@smblee
Copy link
Author

smblee commented May 14, 2021

I was able to reproduce this by using native fetch and adding the same delay call right beforehand.

 await delay(1);
  const apiResponse = await (
    await fetch('api/my-endpoint', {
      headers: { 'Content-Type': 'application/json' },
    })
  ).json();

At this point, it might be an issues i need to take it up to New Relic.. Super strange behavior.

@smblee
Copy link
Author

smblee commented May 14, 2021

I further confirmed that it indeed may be more of an issue with New Relic. They seem to not log AJAX requests when there are other promises happening "around them".

I tried adding a promise right before fetching using libraries that does work with New Relic logging, and immediately, it stopped working.

await delay(1);
await fetch(...);
await delay(1);
await axios(...)
await fetch(...endpoint1);
await fetch(...endpoint2);

none of these were logging to New Relic.

but.. the following works (chaining only axios)

await axios(...)
await axios(...)

and yet the following doesn't:

await delay(1);
await axios(...)
await axios(...)

@sholladay
Copy link
Collaborator

Great debugging, thanks for the details. If you have an idea about how we could remove the delay, we would gladly consider it. But at the moment, I'm not aware of any way to do that without making major changes to the API (which we can also discuss but I think it's unlikely to happen).

Given the above and the fact that this is reproducible without Ky, I'm going to close this issue for now. Will reopen if anything actionable comes up. Depending on New Relic's response, maybe we could provide a way to opt-out of the delay.

lukeupup added a commit to lukeupup/ky that referenced this issue May 25, 2021
@lukeupup
Copy link
Contributor

lukeupup commented May 25, 2021

Hi,

Much thanks to the great work from @smblee. We've encountered exactly the same problem with NewRelic. I found that we can fix the problem by replacing the delay(1) by setTimeout (because it will remove the other promise around the fetch). Although it looks ugly on code level, it might be the most lightweight way to resolve the problem without changing any API of ky.

I will open a PR for this. Hopefully we can reopen this ticket and have further discussion. Thanks.

@lukeupup
Copy link
Contributor

Hi @smblee,
If you can see this message, could you help to test the code in #351? If it can both our problem, I think we can have a relatively easy fix without waiting for new relic. Thank you.

@darrinmn9
Copy link

darrinmn9 commented May 28, 2021

@smblee you're just using the newrelic browser agent SPA right? I saw many open issues about the NR agent not supporting fetch? Am I wrong? I was manually implementing ajax tracking with my app (that uses ky/fetch) and the NR browser agent. Did you have any special configuration? This would be a great relief if simply upgrading ky will fix the NR ajax tracking.

@smblee
Copy link
Author

smblee commented May 28, 2021

@darrinmn9 Their documentation honestly isn't great especially around SPA/Pro browser agent. Some of their pages mention they don't support fetch but that's only for the basic agent. AFAIK from my quick 20 minutes of testing with the fix, everything seemed to be getting shipped up to New Relic as expected (tested with query browser for AjaxRequest table, and their prebuilt dashboard).

That being said, we will try to upgrade to new ky whenever we can get around to it. If you do also and encounter unexpected behaviors please share!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants