feat(feature-flagged-query-cache): Added a feature flagged query cache #19072

asalem1 · 2020-07-24T20:35:53Z

NOTE

This is a preliminary version of the query cache that has a feature flag wrapped around every place where it's being invoked so that we can work off of it. It resolves the windowPeriod issue that created the incident on Friday. This issue was resolved by specifically getting the windowPeriod variable from the query text, a pattern that we use here:

https://github.com/influxdata/influxdb/pull/19072/files#diff-4399ae26b73db6259e86e2dea60efd3dR224-R225

This was added here before running the query:

https://github.com/influxdata/influxdb/pull/19072/files?file-filters%5B%5D=.go&file-filters%5B%5D=.ts&file-filters%5B%5D=.tsx#diff-0be90b2f69a877941cc79979e2747aadR177-R180

What

This PR introduces the concept of a QueryCache to dashboards in order to reduce the number of API requests generated by the UI when toggling between the Cell Configuration Overlay (known as VEO), and the cells on a dashboard.

How

Before each cells runs its respective query, it checks for a matching hashed version of that query in the local cache instance. If one is found, then the data from the cache is returned as promise and runQuery is never run. If one is not found (or if there is a mismatch in variables, or the data is stale), then the operation will continue to runQuery.

Similarly, when the VEO is toggled open, the same operation will occur.

When the cache is instantiated, a watch dog is triggered to clean up any stale data that is currently in the cache.

drdelambre · 2020-07-24T21:01:15Z

ui/src/dashboards/components/DashboardPage.tsx

  public componentWillUnmount() {
-    this.props.resetCachedQueryResults()


doesn't this need to called if the flag is disabled?

Not since the file has been removed and we're no longer caching things. Even if the feature flag is implemented the previous caching system has been ripped out in this PR

that's what i'm trying to call attention to. doesn't that mean that we have interface changes being deployed that aren't captured / contained by the feature flag?

drdelambre · 2020-07-24T21:01:39Z

ui/src/dashboards/components/DashboardPage.tsx

@@ -101,10 +110,6 @@ const mstp = (state: AppState) => {
  }
 }

-const mdtp = {
-  resetCachedQueryResults: resetCachedQueryResults,


and this added back

Not unless we want to integrate the old system back in before we end up removing it later?

drdelambre · 2020-07-24T21:08:48Z

ui/src/shared/apis/queryCache.ts

+    hashedVariables: string,
+    values: RunQueryResult
+  ): void => {
+    event('query_cache_miss', {context: 'queryCache'})


what's the context for?

🤷 I just went based off of what was going on in the other usages. Is there a doc somewhere on event reporting that we should stick to?

we might want to add the queryID into that object so that we can separate parallelized queries

drdelambre · 2020-07-24T21:12:09Z

ui/src/shared/apis/queryCache.ts

+  orgID: string,
+  query: string,
+  state: AppState
+): CancelBox<RunQueryResult> => {


throw an event up here too so that we can split questions like "how many queries did the cache save?" and it'd be queries requested vs query cache set

i think this should be taken care of already in the sense that we already report events for the executeQueries (and TimeSeries.tsx) which this inevitably call this

i dont understand this. feels like an gap between intent vs implementation. explicitly defining the event at the intent (as opposed to using a proxy event rooted in implementation to estimate the intent) helps keep a consistent history to the event, even through implementation changes. if we rewrite all of executeQueries, and forget to put in the eventing, or change the definition of the submodule's eventing structure, our dashboards for cache hits vs missus shouldn't change right? isn't the event you're planning on proxying already invalid because we don't use executeQueries for dashboards?

You're right in that the TimeSeries component does not depend on the executeQueries thunk to run a query, it does, however, a duplicate event. I do see what you're saying though. I'll add it in

drdelambre · 2020-07-28T19:52:12Z

ui/src/shared/apis/queryCache.ts

+  const windowVars = getWindowVars(query, allVars)
+
+  // otherwise query & set results
+  const extern = buildVarsOption([...variableAssignments, ...windowVars])


flagging this for the thread's sake as the fix for the outage... generating a windowPeriod variable from the query and adding it to the AST partial for the query

ebb-tide

it looks good to me..

…hboards

…tion and restructured the way that Cached data was being stored Still need to: - create a filterUnusedVars without a view context to pass into the query - explicitly reset & set the query when the VEO is submitted

… is submitted filtering out unused vars before setting them to query cache

…ta sets used in other tests

…tests

…ay funcs were being imported and updated a few comments

…y & removed comments from tests to reduce confusion

…ry and added a comment to TIME_INVALIDATION const

…unction

… queries

…rence from executeQueries call to cache

…r windowPeriod based on query

…type being used

asalem1 requested a review from drdelambre July 24, 2020 20:35

drdelambre reviewed Jul 24, 2020

View reviewed changes

asalem1 force-pushed the fix/query-cache branch from f40e5dc to af3a35e Compare July 28, 2020 19:44

asalem1 requested review from drdelambre, hoorayimhelping and ebb-tide July 28, 2020 19:44

drdelambre reviewed Jul 28, 2020

View reviewed changes

ebb-tide approved these changes Aug 3, 2020

View reviewed changes

asalem1 requested a review from drdelambre August 3, 2020 22:20

drdelambre approved these changes Aug 3, 2020

View reviewed changes

asalem1 added 17 commits August 4, 2020 06:07

feat(performance): initial stab at creating the caching layer for das…

1774145

…hboards

feat(WIP): WIP

c8fb9dc

feat(query-cache): resets queryCache for corresponding query when VEO…

24f09d0

… is submitted filtering out unused vars before setting them to query cache

chore(test): added tests for filterUnusedVars

3b6566b

feat(test): added tests for the QueryCache and simplified existing da…

fd5bcdb

…ta sets used in other tests

chore(pr-suggestions): added comments, removed unnecessary data from …

4abea72

…tests

chore(filterUnusedVars): removed unused RemoteDataState type

8b03184

chore(ui): removed unused variables from test

5534731

chore(updated-test): updated tests to reduce complexity, update the w…

05c35ad

…ay funcs were being imported and updated a few comments

chore(naming): update method, const and parameter names to add clarit…

53b7c68

…y & removed comments from tests to reduce confusion

chore(naming): renamed getRunQueryResults to getCachedResultsOrRunQue…

2b4d6a7

…ry and added a comment to TIME_INVALIDATION const

chore(thunk): updated to use a thunk rather than passing the entire f…

eeff92d

…unction

chore(TIME_INVALIDATION): updated the invalidation time to ten minutes

d91d39b

chore(watchdog): running watchdog every 5 minutes

67cdcf7

chore(rebase): rebased master

64e4d95

feat(integrated-mutex): integrated mutex and state initialization for…

aafbd09

… queries

asalem1 added 8 commits August 4, 2020 06:13

chore(rebase-master): rebased master and removed abortController refe…

ed30f2d

…rence from executeQueries call to cache

feat(flag): added feature flag to queryCache

5f694d0

chore(event): added eventing for getting and setting on the queryCache

99aa8e5

chore(event-naming): renamed event strings to be more readable

e8ff1cf

chore(update-metrics): updated event metrics

3823e86

chore(event): added an event to register initial query cache events

3776c3d

fix(windowPeriod): fixed windowPeriod issue by separately querying fo…

f20ce7b

…r windowPeriod based on query

chore(ts-issue): resolved TS issue that was caused by wrong variable …

aeb14b9

…type being used

asalem1 force-pushed the fix/query-cache branch from 6b5bebd to aeb14b9 Compare August 4, 2020 13:18

asalem1 merged commit fdea2c6 into master Aug 4, 2020

asalem1 deleted the fix/query-cache branch August 4, 2020 13:56

asalem1 mentioned this pull request Aug 4, 2020

Don't reload query when closing a cell #18455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(feature-flagged-query-cache): Added a feature flagged query cache #19072

feat(feature-flagged-query-cache): Added a feature flagged query cache #19072

asalem1 commented Jul 24, 2020 •

edited

Loading

drdelambre Jul 24, 2020

asalem1 Jul 24, 2020

drdelambre Jul 27, 2020

drdelambre Jul 24, 2020

asalem1 Jul 24, 2020

drdelambre Jul 24, 2020

asalem1 Jul 24, 2020

drdelambre Jul 24, 2020

drdelambre Jul 24, 2020

asalem1 Jul 24, 2020 •

edited

Loading

drdelambre Jul 27, 2020

asalem1 Jul 27, 2020

drdelambre Jul 28, 2020

ebb-tide Aug 3, 2020

ebb-tide left a comment

		public componentWillUnmount() {
		this.props.resetCachedQueryResults()

feat(feature-flagged-query-cache): Added a feature flagged query cache #19072

feat(feature-flagged-query-cache): Added a feature flagged query cache #19072

Conversation

asalem1 commented Jul 24, 2020 • edited Loading

What

How

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalem1 Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebb-tide left a comment

Choose a reason for hiding this comment

asalem1 commented Jul 24, 2020 •

edited

Loading

asalem1 Jul 24, 2020 •

edited

Loading