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

feat: refactor cache handling, move storage out of generated #238

Merged
merged 22 commits into from Oct 11, 2022

Conversation

fiam
Copy link
Contributor

@fiam fiam commented Oct 5, 2022

  • feat: move introspection cache to .wundergraph/introspection/cache
  • feat: prioritize cache during introspection

Checklist

@vercel
Copy link

vercel bot commented Oct 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
wundergraph-docs ⬜️ Ignored (Inspect) Oct 11, 2022 at 3:50PM (UTC)

@fiam fiam force-pushed the alberto/eng-465-introspection-should-work-just-by-using branch from 96efdba to bdbd5f9 Compare October 6, 2022 10:27
@fiam
Copy link
Contributor Author

fiam commented Oct 6, 2022

Made the requested changed and rebased

@fiam fiam changed the title feat: prioritize introspection cache, move out of generated feat: refactor cache handling, move storage out of generated Oct 6, 2022
cli/commands/root.go Outdated Show resolved Hide resolved
cli/commands/generate.go Outdated Show resolved Hide resolved
cli/commands/up.go Outdated Show resolved Hide resolved
cli/commands/root.go Outdated Show resolved Hide resolved
cli/commands/up.go Outdated Show resolved Hide resolved
cli/commands/up.go Show resolved Hide resolved
cli/commands/up.go Outdated Show resolved Hide resolved
cli/commands/up.go Outdated Show resolved Hide resolved
@fiam fiam force-pushed the alberto/eng-465-introspection-should-work-just-by-using branch from 4b16bc6 to 56eed6a Compare October 7, 2022 14:23
@jensneuse
Copy link
Member

One thing I'd add is to set all "introspection" configs to automatically refresh each cache item every 5 seconds if not otherwise configured. So, by default, all introspections should be cached and refreshed after 5 seconds of waiting. This will give the most pleasent DX as it will be fast and (almost) always up to date if a datasource changes. @StarpTech opinion?

@fiam fiam force-pushed the alberto/eng-465-introspection-should-work-just-by-using branch from 56eed6a to 5c2243e Compare October 7, 2022 14:51
@fiam
Copy link
Contributor Author

fiam commented Oct 7, 2022

One thing I'd add is to set all "introspection" configs to automatically refresh each cache item every 5 seconds if not otherwise configured. So, by default, all introspections should be cached and refreshed after 5 seconds of waiting. This will give the most pleasent DX as it will be fast and (almost) always up to date if a datasource changes. @StarpTech opinion?

So basically making pollingIntervalSeconds default to 5 while running from wunderctl up?

@jensneuse
Copy link
Member

One thing I'd add is to set all "introspection" configs to automatically refresh each cache item every 5 seconds if not otherwise configured. So, by default, all introspections should be cached and refreshed after 5 seconds of waiting. This will give the most pleasent DX as it will be fast and (almost) always up to date if a datasource changes. @StarpTech opinion?

So basically making pollingIntervalSeconds default to 5 while running from wunderctl up?

It can always default to 5s, not just for "up", because when using "start", we don't introspect.

@StarpTech
Copy link
Collaborator

StarpTech commented Oct 7, 2022

Assumption: Most people integrate a ready API. They don't need to refresh it so often. If so, they can configure it. For me, it makes more sense to opt in. It produces fewer errors and logs.

@jensneuse
Copy link
Member

Assumption: Most people integrate an ready API into the service. They don't need to refresh it so often. If so, they can configure it. For me, it makes more sense to opt in.

ok, makes sense

@fiam
Copy link
Contributor Author

fiam commented Oct 7, 2022

@StarpTech GH says there's one remaining change requested from you, but then it links to a resolved one. Can we dismiss it?

@jensneuse
Copy link
Member

@StarpTech GH says there's one remaining change requested from you, but then it links to a resolved one. Can we dismiss it?

@StarpTech if you're ok, please approve

cli/commands/generate.go Outdated Show resolved Hide resolved
@StarpTech
Copy link
Collaborator

I'd like to test it once locally.

@fiam fiam force-pushed the alberto/eng-465-introspection-should-work-just-by-using branch from 5079604 to 687bd27 Compare October 7, 2022 20:18
@jensneuse
Copy link
Member

Once the implementation is done, the docs need updates as well:
https://docs.wundergraph.com/docs/wundergraph-config-ts-reference/configure-introspection

Always try to hit the cache first. Once a result is retrieved, populate
the cache as long as it's not disabled. Make the cache enabled by
default, but toggleable via WG_DISABLE_INTROSPECTION_CACHE.
This way we can eventually place more items into a cache using the
hierarchy below $WGD/cache
wunderctl generate was not propertly initializing it. Instead of making
commands that might end up using the cache initialize the directory,
make the TS side initialize it lazily when needed.
Instead of testing for file existence, try to read it and handle the
potential errors.
- New --cache flag enables caching, which otherwise is always disabled
  by default
- --cache is implemented globally for all commands, although it doesn't
  have any effect for some of them
- TS side honors this via WG_ENABLE_INTROSPECTION_CACHE environment
  variable
- Enable cache by default
- During wunderctl generate, fail if there's a cache miss
- Add --cache-fallback flag to wunderctl generate, which allows falling
  back onto introspection
- Pass WG_ENABLE_INTROSPECTION_CACHE explicitely instead of setting it
  globally
- Clear the cache by default during wunderctl up startup
- Add a configurable time interval to clear the cache during wunderctl
  up, with a 5 seconds default
Let the TS side do the refreshes when polling is enabled
Make it clear that the error is because offline mode is enabled, include
details of the introspection that failed
As discussed with Dustin, this is useful for generating a cache from a
known and cold state, to reuse it later (e.g. for CI runs)
Added to both .gitignore and .prettierignore, as well as testsapps and
examples
Copy link
Collaborator

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fiam fiam enabled auto-merge (squash) October 11, 2022 14:45
@fiam fiam force-pushed the alberto/eng-465-introspection-should-work-just-by-using branch 2 times, most recently from 0e97054 to 46f1b21 Compare October 11, 2022 15:26
5 minutes is too slow for testapps/default in CI, it ends up being
cancelled. Investigate this a bit further, because it shouldn't
take this long.
@fiam fiam force-pushed the alberto/eng-465-introspection-should-work-just-by-using branch from 46f1b21 to 1592765 Compare October 11, 2022 15:50
@fiam fiam merged commit 6351e35 into main Oct 11, 2022
@fiam fiam deleted the alberto/eng-465-introspection-should-work-just-by-using branch October 11, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants