insights: persist patternType in db#63579
Conversation
This PR refactors insights to support the upcoming Keyword Search GA. New insights are persisted with `patternType:` appended to query. Existing insights are not affected. Test plan: - new unit test - existing tests pass without changes
| query: values.seriesQuery, | ||
| query: hasPatternType(values.seriesQuery) | ||
| ? values.seriesQuery | ||
| : `patterntype:${defaultPatternType} ${values.seriesQuery}`, |
There was a problem hiding this comment.
Putting "patterntype:" in front of the query matches what we do in "Detect and track patterns" (see here). Code Monitors and Search Jobs, for example, append the pattern type.
| export type InsightDataSeriesData = Pick<InsightDataSeries, 'seriesId' | 'label' | 'points'> | ||
|
|
||
| export function generateLinkURL(query: string | null): string | undefined { | ||
| export function generateLinkURL(query: string | null, defaultPatternType: SearchPatternType): string | undefined { |
There was a problem hiding this comment.
This caused a lot of other changes because I had to propagate the default pattern type until I hit the first React FC.
jtibshirani
left a comment
There was a problem hiding this comment.
This PR looks good to me overall.
New insights are persisted with patternType: appended to query. Existing insights are not affected... Execution of insights relies on the defaults of the Stream API (currently "V3"). This means existing code insights without pattern type will keep executing with "standard" pattern type until we update the API.
It feels like we should migrate existing insights too. Otherwise, what will we do in "Phase 2" when we introduce V4 and no longer want to default to V3? Also this would match what we did for notebooks.
3f1b51b to
7528c9c
Compare
For Phase 1 it is not really necessary to migrate the queries. The PR works as-is for GA. Even for Phase 2 we can either migrate the old queries or update the call of Insights to the Stream API and set "V3". Based on your comment, though, I assume you prefer to migrate the queries ;-) I added a migration which updates the queries. PTAL |
jtibshirani
left a comment
There was a problem hiding this comment.
We discussed offline and agreed the migration was nice, and not too risky. This way, there's a consistency across all stored data, and we don't need to worry about fixing the version type to V3.
| UPDATE insight_series SET query_old = query; | ||
|
|
||
| -- prefix query with patternType:standard if there is no patterntype: in the query | ||
| UPDATE insight_series |
There was a problem hiding this comment.
Looks good! Nice to learn how revert this type of migration.
This PR refactors insights to support the upcoming Keyword Search GA.
New insights are always persisted with
patternType:.Queries of existing insights are updated with
patternType:standard <query>if they don't already specify a pattern type. This reflects the current default of the Stream API and thus, merely makes the pattern type explicit.Notes:
Test plan: