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

Mechanism to detect non-200 responses from Workbox #1391

Closed
josephliccini opened this issue Mar 26, 2018 · 6 comments
Closed

Mechanism to detect non-200 responses from Workbox #1391

josephliccini opened this issue Mar 26, 2018 · 6 comments

Comments

@josephliccini
Copy link
Contributor

Library Affected:
workbox-sw

Browser & Platform:
Any

Issue or Feature Request Description:
Hello again!

By Fetch API specification, HTTP statuses of any response (whether it be a 200, 300, 400, or 500) will not be considered an exception, unless the request actually fails to send (which makes sense).

Our web application on a large scale calls into various APIs and we use telemetry to monitor the health of each API we call into. One dimension we would do this on is statusCode. That way, we can see spikes / outages for various APIs.

Now, with a SW, it's possible that the API could be failing, but we serve up stale data (which is awesome). However, we would like a way to continue to log the reliability of the WhileRevalidate part of the process, because now, any cached responses that get served will now blind our application from detecting API failures.

What is the best way to go about this? Should this be supported in workbox? Could the CatchHandler also intercept requests that have a certain response code? I think (right now) the best way to go about it is to wrap all of our handlers in another handler that will inspect the status code of the revalidation response. (I haven't started creating that handler yet, so I'm not sure it would even work. Will be doing that shortly)

Thanks!

When reporting bugs, please include relevant JavaScript Console logs and links to public URLs at which the issue could be reproduced.

@gauntface
Copy link

My gut is says that this should be something supported via a plugin.

I think adding an additional lifecycle event for fetchDidSucceed seems the obvious place a fetched response can be tracked or intercepted and changed. https://developers.google.com/web/tools/workbox/guides/using-plugins

@josephliccini
Copy link
Contributor Author

josephliccini commented Mar 26, 2018

Here is what I've came up with (though it's kind of a hack):
(NOTE: I did edit this with an implementation that is not as unstable, but I think it's still a hack :) )

// This is a custom workbox strategy that implements the 'stale-while-revalidate' cache pattern:
// https://developers.google.com/web/fundamentals/instant-and-offline/offline-cookbook/#stale-while-revalidate
// It also emits instrumentation events via `postMessage` to the source of the requests, emitting errors
const instrumentedStaleWhileRevalidate = (staleWhileRevalidateConfig) => {
    return {
        handle: ({ event }) => {
            const _self = self;

            const handler = workboxSW.strategies.staleWhileRevalidate(staleWhileRevalidateConfig);

            const requestWrapper = handler.requestWrapper;
            const originalFetch = handler.requestWrapper.fetch.bind(requestWrapper);

            const wrappedFetch = async ({ request }) => {
                const response = await originalFetch({ request });

                const clients = await _self.clients.matchAll();
                const client = clients.filter(client => client.id === event.clientId)[0];

                if (response.ok) {
                    // TODO: Emit REQUEST_SUCCESS message to client, to measure revalidation time
                    return response;
                }

                const result = {
                    type: 'REQUEST_FAILURE',
                    meta: 'workbox-request-failure-message',
                    payload: {
                        url: request.url,
                        cacheName: staleWhileRevalidateConfig.cacheName,
                        status: response.status, // HTTP Status Code
                        data: await response.clone().text() // We can only ready body of response once; we need to clone it before writing to message
                    }
                };

                if (client) {
                    client.postMessage(result);
                }

                return response;
            };

            requestWrapper.fetch = wrappedFetch;
            return handler.handle({ event });
        }
    }
};

For our case, we need to notify the client that the failure has happened, so we have the additional constraint of needing the request and the event so we can notify the client.

@jeffposnick
Copy link
Contributor

Adding a new fetchDidSucceed would be the most direct interface, but a slightly more indirect route (but one that's currently supported) would be to write your own cacheWillUpdate plugin.

Your plugin code will be passed in the fresh Response from the network, and it will be called automatically before the cache update that happens "under the hood" during stale-while-revalidate.

Workbox supports defining multiple cacheWillUpdate plugins for a given strategy. If you're already using workbox.cacheableResponse (which also implements cacheWillUpdate), you should list your own custom plugin first, to ensure that you're always passed the Response from the network before workbox.cacheableResponse has a chance to filter it out.

const instrumentationPlugin = {
  cacheWillUpdate: async ({request, response}) => {
    // Do your instrumentation, making use of request and/or response.

    // Make sure to return the original response!
    return response;
  },
};

workbox.routing.registerRoute(
  new RegExp('/api/'),
  workbox.strategies.staleWhileRevalidate({
    cacheName: 'api-responses',
    plugins: [
      instrumentationPlugin,
      // ...any other plugins...
    ],
  })
);

@jeffposnick jeffposnick changed the title Mechanism to detect non-200 requests from Workbox Mechanism to detect non-200 responses from Workbox Mar 27, 2018
@josephliccini
Copy link
Contributor Author

@jeffposnick cool! I will give it a try. I am on workbox 2.1.3, and I have been getting exceptions that disallow multiple 'cacheWillUpdate' plugins, but I haven't upgraded to 3.X yet.

Though this implementation you have linked, I couldn't postMessage to the client that sent the request. But I do think long term we should be doing telemetry directly from the worker, and not relying on the app to process telemetry.

@chrisdarroch
Copy link

chrisdarroch commented Jun 19, 2018

I wanted to +1 the original request.

One point of order: the use-case described is almost exactly the same as the HTTP Cache-Control stale-if-error strategy: https://tools.ietf.org/html/rfc5861#section-3. One key difference is that the RFC defines an "error" as a specific set of 5xx HTTP responses; it would still treat 3xx and 4xx responses as valid.

While I appreciate that the effect can technically be achieved using a combination of staleWhileRevalidate with a custom cacheWillUpdate in front of a configured workbox.cacheableResponse... it would be a boon for workbox to implement an equivalent callback and abstract this strategy.

I'm happy to split this in to a separate issue / request if you think it's a sufficiently different use-case.

@jeffposnick
Copy link
Contributor

We should be able to address this issue via a new fetchDidSucceed callback in v4: #1772

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

No branches or pull requests

5 participants