apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.38k stars 2.66k forks source link

HTTP link "fetcher strategy" circumvents common monitoring techniques #7832

Closed bripkens closed 3 years ago

bripkens commented 3 years ago

Disclaimer: I am working for Instana, an monitoring/application-performance management company. I am opening this bug ticket as a result of several issues our customers are raising with us due to the HTTP link implementation. The intention is an upstream fix so that our customers, but also the customers of our competition, encounter fewer pitfalls when leveraging our solutions in combination with Apollo's HTTP link for an overall better end-user experience.

Intended outcome: HTTP calls executed via HTTP link should be reliably monitored by application-performance management solutions, e.g. Instana, Speedcurve, New Relic…

Actual outcome: HTTP calls cannot be reliably monitored. Only when the monitoring solution gets executed before the HTTP link gets created, monitoring works reliably. This happens because window.fetch is assigned to a local variable during the HTTP link creation. This in turn means that instrumentations of window.fetch by monitoring solutions may (not) be able to monitor window.fetch usages of Apollo HTTP link (loading order dependence is introduced).

Proposed solution: Implement the fallback to window.fetch when making HTTP calls and not when creating the HTTP link, e.g. (fetcher || fetch)!(chosenURI, options).

How to reproduce the issue: Overwrite window.fetch after Apollo HTTP link is loaded and notice that Apollo HTTP link will not leverage this updated window.fetch value.

Versions Latest

assertnotnull commented 3 years ago

I also believe this problem is causing monitoring problems with https://github.com/DataDog/browser-sdk/tree/main/packages/rum and impedes us to see actions linked to backend logs and traces.

mlevkovsky commented 3 years ago

any update on this? We want to be able to instrument our frontend and backend applications, but this seems to be a hard blocker

benjamn commented 3 years ago

@bripkens @mlevkovsky @assertnotnull Fix incoming: #8603. Will that PR enable your instrumentation use cases?