graphql-mesh icon indicating copy to clipboard operation
graphql-mesh copied to clipboard

Improve docs for `customFetch` usage

Open ardatan opened this issue 2 years ago • 6 comments

ardatan avatar Jun 30 '22 13:06 ardatan

@Urigo Could you please put it here at least link to test suite to see how customFetch can be used? Or a quick example? This blocks us of upgrading the graphql-mesh :-(

I'm trying it but the problem I see is that the function receives the request object instead of url + opts. Then e.g. node-fetch doesn't work at all.

lkrzyzanek avatar Sep 07 '22 08:09 lkrzyzanek

@lkrzyzanek Why do you want to use a customFetch? Actually the signature is like that; https://github.com/Urigo/graphql-mesh/blob/master/packages/types/src/index.ts#L162

ardatan avatar Sep 07 '22 11:09 ardatan

We started using to node-fetch because Undici doesn't have working OpenTelemtry instrumentation. In our architecture this plays significant role. Then our custom fetcher is very fine tuned especially in terms of keep-alive configuration, retry mechanism, error handling etc.

This method Signature looks great ! But in our setup it behaves very differently. I receive just 1 argument and it's Request object. I was debugging it and it comes from fetchache module - pasting the place where fetch is called then:

function fetchFactory({ fetch, Request, Response, cache }) {
    return async (input, init) => {
        let request;
        if (input instanceof Request) {
            request = input;
        }
        else {
            request = new Request(input, init);
        }
        const cacheKey = request.url;
        const entry = await cache.get(cacheKey);
        if (!entry) {
            const response = await fetch(request);

See here: https://github.com/ardatan/whatwg-node/blob/%40whatwg-node/server%400.1.2/packages/fetchache/src/index.ts#L23

@ardatan Thank you for your reply!

lkrzyzanek avatar Sep 07 '22 12:09 lkrzyzanek

@lkrzyzanek You are right. It's been fixed in fetchache now. It will be released on Mesh soon. https://github.com/ardatan/whatwg-node/pull/104

By the way we recently introduced onFetch hook that allows you to manipulate HTTP fetch calls and we started using it in some tracing plugins; https://github.com/Urigo/graphql-mesh/blob/master/packages/plugins/prometheus/src/index.ts All other plugins are here; https://www.the-guild.dev/graphql/mesh/docs/guides/monitoring-and-tracing

We couldn't find enough time for OpenTelemetry. About retry, timeout mechanism, we have implemented it in GraphQL Handler; https://www.the-guild.dev/graphql/mesh/docs/handlers/graphql#fetch-strategies-and-multiple-http-endpoints-for-the-same-source But we would love to implement it for other handlers too.

ardatan avatar Sep 07 '22 13:09 ardatan

Also have you seen this? https://github.com/gadget-inc/opentelemetry-instrumentations/tree/main/packages/opentelemetry-instrumentation-undici

ardatan avatar Sep 07 '22 16:09 ardatan

Thanks for such quick fix. Much appreciated.

onFetch sounds like a great idea. Thanks for links. I'll take a look on that closer later.

I'm aware of retry/timeout/fallback strategies. They are great for basic usage. However the retry is not much configurable. It's just a number. Take a look here: https://github.com/jonbern/fetch-retry - Features like retryDelay or retryOn are needed as well. For example when the DS returns 400 - there is no need to perform retry. Or delay between retries are also very great practise.

Also have you seen this? https://github.com/gadget-inc/opentelemetry-instrumentations/tree/main/packages/opentelemetry-instrumentation-undici

Yes. Unfortunately it didn't work for us and given that it's not official otel plugin + we switched to node-fetch

lkrzyzanek avatar Sep 08 '22 07:09 lkrzyzanek

Related to needing docs, is there anyway to specify customFetch on a per source basis or can it only be specified globally?

Attempting this

sources:
  - name: MySource
    handler:
      openapi:
        customFetch: ./src/custom-fetch.ts

gives this in the logs:

⚠️ 🕸️  Mesh - config Configuration file is not valid!
⚠️ 🕸️  Mesh - config This is just a warning! It doesn't have any effects on runtime.
⚠️ 🕸️  Mesh - config must NOT have additional properties

cancan101 avatar Sep 27 '22 02:09 cancan101

@cancan101 No there is not. You can write a plugin with onFetch hook for custom configuration.

@lkrzyzanek PRs are welcome!

ardatan avatar Sep 27 '22 09:09 ardatan