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

Improve docs for customFetch usage #4110

Open
ardatan opened this issue Jun 30, 2022 · 8 comments
Open

Improve docs for customFetch usage #4110

ardatan opened this issue Jun 30, 2022 · 8 comments

Comments

@ardatan
Copy link
Owner

ardatan commented Jun 30, 2022

No description provided.

@Urigo Urigo mentioned this issue Aug 11, 2022
46 tasks
@lkrzyzanek
Copy link

@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.

@ardatan
Copy link
Owner Author

ardatan commented Sep 7, 2022

@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

@lkrzyzanek
Copy link

lkrzyzanek commented Sep 7, 2022

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!

@ardatan
Copy link
Owner Author

ardatan commented Sep 7, 2022

@lkrzyzanek You are right. It's been fixed in fetchache now. It will be released on Mesh soon.
ardatan/whatwg-node#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
Copy link
Owner Author

ardatan commented Sep 7, 2022

@lkrzyzanek
Copy link

lkrzyzanek commented Sep 8, 2022

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

@cancan101
Copy link

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

@ardatan
Copy link
Owner Author

ardatan commented Sep 27, 2022

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

@lkrzyzanek PRs are welcome!

This was referenced Apr 30, 2024
This was referenced May 7, 2024
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

No branches or pull requests

3 participants