Skip to content

Commit

Permalink
feat(insights): Make initial single insight load async v2 (PostHog#23978
Browse files Browse the repository at this point in the history
)

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Anirudh Pillai <[email protected]>
  • Loading branch information
3 people authored and silentninja committed Aug 8, 2024
1 parent 1341b02 commit e73d95b
Show file tree
Hide file tree
Showing 22 changed files with 502 additions and 49 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci-e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ jobs:
OBJECT_STORAGE_SECRET_ACCESS_KEY=object_storage_root_password
GITHUB_ACTION_RUN_URL="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}"
CELERY_METRICS_PORT=8999
CLOUD_DEPLOYMENT=1
CLOUD_DEPLOYMENT=E2E
EOT
- name: Start PostHog
Expand Down
2 changes: 1 addition & 1 deletion .run/Celery Threads.run.xml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<option name="ADD_SOURCE_ROOTS" value="true" />
<EXTENSION ID="PythonCoverageRunConfigurationExtension" runner="coverage.py" />
<option name="SCRIPT_NAME" value="$PROJECT_DIR$/manage.py" />
<option name="PARAMETERS" value="run_autoreload_celery" />
<option name="PARAMETERS" value="run_autoreload_celery --type=worker" />
<option name="SHOW_COMMAND_LINE" value="false" />
<option name="EMULATE_TERMINAL" value="false" />
<option name="MODULE_MODE" value="false" />
Expand Down
2 changes: 1 addition & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
},
"request": "launch",
"program": "${workspaceFolder}/manage.py",
"args": ["run_autoreload_celery"],
"args": ["run_autoreload_celery", "--type=worker"],
"console": "integratedTerminal",
"python": "${workspaceFolder}/env/bin/python",
"cwd": "${workspaceFolder}",
Expand Down
13 changes: 7 additions & 6 deletions bin/e2e-test-runner
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,13 @@ do
fi
done

export DEBUG=1
export NO_RESTART_LOOP=1
export CYPRESS_BASE_URL=http://localhost:8080
export SECURE_COOKIES=0
export SKIP_SERVICE_VERSION_REQUIREMENTS=1
export KAFKA_HOSTS=kafka:9092
export CLICKHOUSE_DATABASE=posthog_test
export TEST=1 # Plugin server and kafka revert to 'default' Clickhouse database if TEST is not set
export CLICKHOUSE_SECURE=0
export JS_URL=http://localhost:8234
export E2E_TESTING=1
export SECRET_KEY=e2e_test
export EMAIL_HOST=email.test.posthog.net
Expand All @@ -66,7 +64,9 @@ export PGUSER="${PGUSER:=posthog}"
export PGPASSWORD="${PGPASSWORD:=posthog}"
export PGPORT="${PGPORT:=5432}"
export DATABASE_URL="postgres://${PGUSER}:${PGPASSWORD}@${PGHOST}:${PGPORT}/${DATABASE}"
export CLOUD_DEPLOYMENT=1
export CLOUD_DEPLOYMENT=E2E

source ./bin/celery-queues.env

trap "trap - SIGTERM && kill -- -$$" SIGINT SIGTERM EXIT

Expand All @@ -92,8 +92,7 @@ setupDev() {
python manage.py setup_dev &
}

nc -z localhost 9092 || ( echo -e "\033[0;31mKafka isn't running. Please run\n\tdocker compose -f docker-compose.dev.yml up zookeeper kafka clickhouse db redis\nI'll wait while you do that.\033[0m" ; bin/check_kafka_clickhouse_up )
wget -nv -t1 --spider 'http://localhost:8123/' || ( echo -e "\033[0;31mClickhouse isn't running. Please run\n\tdocker compose -f docker-compose.dev.yml up zookeeper kafka clickhouse db redis.\nI'll wait while you do that.\033[0m" ; bin/check_kafka_clickhouse_up )
bin/check_kafka_clickhouse_up

$SKIP_RECREATE_DATABASE || recreateDatabases
$SKIP_MIGRATE || migrateDatabases
Expand All @@ -103,4 +102,6 @@ $SKIP_SETUP_DEV || setupDev
# Only start webpack if not already running
nc -vz 127.0.0.1 8234 2> /dev/null || ./bin/start-frontend &
pnpm dlx cypress open --config-file cypress.e2e.config.ts &
uv pip install -r requirements.txt -r requirements-dev.txt
python manage.py run_autoreload_celery --type=worker &
python manage.py runserver 8080
4 changes: 2 additions & 2 deletions bin/start-worker
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ trap 'kill $(jobs -p)' EXIT
source ./bin/celery-queues.env

# start celery worker with heartbeat (-B)
SKIP_ASYNC_MIGRATIONS_SETUP=0 CELERY_WORKER_QUEUES=$CELERY_WORKER_QUEUES celery -A posthog worker --without-heartbeat --without-mingle --pool=threads -Ofair -n node@%h &
celery -A posthog beat -S redbeat.RedBeatScheduler &
python manage.py run_autoreload_celery --type=worker &
python manage.py run_autoreload_celery --type=beat &

if [[ "$PLUGIN_SERVER_IDLE" != "1" && "$PLUGIN_SERVER_IDLE" != "true" ]]; then
./bin/plugin-server
Expand Down
13 changes: 7 additions & 6 deletions cypress.e2e.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,16 @@ export default defineConfig({
const redisClient = await createClient()
.on('error', (err) => console.log('Redis client error', err))
.connect()
for await (const key of redisClient.scanIterator({
TYPE: 'string',
MATCH: '*cache*',
COUNT: 100,
})) {
// Clear cache
for await (const key of redisClient.scanIterator({ TYPE: 'string', MATCH: '*cache*', COUNT: 500 })) {
await redisClient.del(key)
}
// Also clear the more ephemeral async query statuses
for await (const key of redisClient.scanIterator({ TYPE: 'string', MATCH: 'query_async*', COUNT: 500 })) {
await redisClient.del(key)
}
await redisClient.quit()
return null
return null // Cypress requires _some_ return value
},
})

Expand Down
40 changes: 40 additions & 0 deletions cypress/e2e/dashboard.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,46 @@ describe('Dashboard', () => {
}
})

it('Refreshing dashboard works', () => {
const dashboardName = randomString('Dashboard with insights')
const insightName = randomString('insight to add to dashboard')

// Create and visit a dashboard to get it into turbo mode cache
dashboards.createAndGoToEmptyDashboard(dashboardName)

insight.create(insightName)

insight.addInsightToDashboard(dashboardName, { visitAfterAdding: true })

cy.get('.CardMeta h4').should('have.text', insightName)
cy.get('h4').contains('Refreshing').should('not.exist')
cy.get('main').contains('There are no matching events for this query').should('not.exist')

cy.intercept('GET', /\/api\/projects\/\d+\/dashboard_templates/, (req) => {
req.reply((response) => {
response.body.results[0].variables = [
{
id: 'id',
name: 'Unique variable name',
type: 'event',
default: {},
required: true,
description: 'description',
},
]
return response
})
})

// refresh the dashboard by changing date range
cy.get('[data-attr="date-filter"]').click()
cy.contains('span', 'Last 14 days').click()
cy.contains('span', 'Apply and save dashboard').click()

cy.contains('span[class="text-primary text-sm font-medium"]', 'Refreshing').should('not.exist')
cy.get('span').contains('Refreshing').should('not.exist')
})

it('Shows details when moving between dashboard and insight', () => {
const dashboardName = randomString('Dashboard')
const insightName = randomString('DashboardInsight')
Expand Down
44 changes: 44 additions & 0 deletions cypress/e2e/insights-saved.cy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { urls } from 'scenes/urls'

import { createInsight } from '../productAnalytics'

chai.Assertion.addMethod('neverHaveChild', function (childSelector) {
this._obj.on('DOMNodeInserted', () => {
const matchCount = cy.$$(childSelector, this._obj).length
if (matchCount > 0) {
throw new Error(
`Expected element to never have child ${childSelector}, but found ${matchCount} match${
matchCount > 1 ? 'es' : ''
}`
)
}
})
})

// For tests related to trends please check trendsElements.js
// insight tests were split up because Cypress was struggling with this many tests in one file🙈
describe('Insights - saved', () => {
it('Data is available immediately', () => {
createInsight('saved insight').then((newInsightId) => {
cy.get('[data-attr=trend-line-graph]').should('exist') // Results cached
cy.visit(urls.insightView(newInsightId)) // Full refresh
cy.get('.InsightViz').should('exist').should('neverHaveChild', '.insight-empty-state') // Only cached data
cy.get('[data-attr=trend-line-graph]').should('exist')
})
})

it('If cache empty, initiate async refresh', () => {
cy.intercept('GET', /\/api\/projects\/\d+\/insights\/?\?[^/]*?refresh=async/).as('getInsightsRefreshAsync')
let newInsightId: string
createInsight('saved insight').then((insightId) => {
newInsightId = insightId
})
cy.task('resetInsightCache').then(() => {
cy.visit(urls.insightView(newInsightId)) // Full refresh
cy.get('.insight-empty-state').should('exist') // There should be a loading state for a moment
cy.wait('@getInsightsRefreshAsync').then(() => {
cy.get('[data-attr=trend-line-graph]').should('exist')
})
})
})
})
6 changes: 5 additions & 1 deletion cypress/productAnalytics/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,15 @@ export const dashboard = {
},
}

export function createInsight(insightName: string): void {
export function createInsight(insightName: string): Cypress.Chainable<string> {
savedInsights.createNewInsightOfType('TRENDS')
insight.applyFilter()
insight.editName(insightName)
insight.save()
// return insight id from the url
return cy.url().then((url) => {
return url.split('/').at(-1)
})
}

export function duplicateDashboardFromMenu(duplicateTiles = false): void {
Expand Down
4 changes: 4 additions & 0 deletions cypress/support/e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ Cypress.on('window:before:load', (win) => {
win._cypress_posthog_captures = []
})

before(() => {
cy.task('resetInsightCache') // Reset insight cache before each suite
})

beforeEach(() => {
Cypress.env('POSTHOG_PROPERTY_CURRENT_TEST_TITLE', Cypress.currentTest.title)
Cypress.env('POSTHOG_PROPERTY_CURRENT_TEST_FULL_TITLE', Cypress.currentTest.titlePath.join(' > '))
Expand Down
6 changes: 4 additions & 2 deletions frontend/src/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -899,14 +899,16 @@ const api = {
insights: {
loadInsight(
shortId: InsightModel['short_id'],
basic?: boolean
basic?: boolean,
refresh?: RefreshType
): Promise<PaginatedResponse<Partial<InsightModel>>> {
return new ApiRequest()
.insights()
.withQueryString(
toParams({
short_id: encodeURIComponent(shortId),
basic: basic,
basic,
refresh,
})
)
.get()
Expand Down
14 changes: 10 additions & 4 deletions frontend/src/queries/nodes/DataNode/dataNodeLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,14 @@ export const dataNodeLogic = kea<dataNodeLogicType>([
if (oldProps.query?.kind && props.query.kind !== oldProps.query.kind) {
actions.clearResponse()
}
if (
const hasQueryChanged = !queryEqual(props.query, oldProps.query)
const queryStatus = (props.cachedResults?.query_status || null) as QueryStatus | null
if (hasQueryChanged && queryStatus?.complete === false) {
// If there is an incomplete query, load the data with the same query_id which should return its status
actions.loadData(undefined, queryStatus.id)
} else if (
hasQueryChanged &&
!(props.cachedResults && props.key.includes('dashboard')) && // Don't load data on dashboard if cached results are available
!queryEqual(props.query, oldProps.query) &&
(!props.cachedResults ||
(isInsightQueryNode(props.query) && !props.cachedResults['result'] && !props.cachedResults['results']))
) {
Expand All @@ -141,7 +146,7 @@ export const dataNodeLogic = kea<dataNodeLogicType>([
}
}),
actions({
loadData: (refresh = false) => ({ refresh, queryId: uuid() }),
loadData: (refresh = false, queryId?: string) => ({ refresh, queryId: queryId || uuid() }),
abortAnyRunningQuery: true,
abortQuery: (payload: { queryId: string }) => payload,
cancelQuery: true,
Expand All @@ -166,7 +171,8 @@ export const dataNodeLogic = kea<dataNodeLogicType>([
return props.cachedResults
}

if (props.cachedResults && !refresh) {
const queryStatus = (props.cachedResults?.query_status || null) as QueryStatus | null
if (props.cachedResults && !refresh && queryStatus?.complete !== false) {
if (props.cachedResults['result'] || props.cachedResults['results']) {
return props.cachedResults
}
Expand Down
Loading

0 comments on commit e73d95b

Please sign in to comment.