From 3c860b3668db2ebe54147fb36111db56fa975348 Mon Sep 17 00:00:00 2001
From: Elwyn Benson <ebenson@gitlab.com>
Date: Mon, 31 Jul 2023 10:12:02 +0000
Subject: [PATCH] Remove extra handling of bad data in SingleStats
Malformed query / response should trigger an error instead of silently
failing and falling back to 0
---
.../data_sources/cube_analytics.js | 17 +-
.../instrumentation_instructions.vue | 22 +--
.../data_sources/cube_analytics_spec.js | 159 ++++++++++--------
.../analytics_dashboards/mock_data.js | 10 ++
4 files changed, 110 insertions(+), 98 deletions(-)
diff --git a/ee/app/assets/javascripts/analytics/analytics_dashboards/data_sources/cube_analytics.js b/ee/app/assets/javascripts/analytics/analytics_dashboards/data_sources/cube_analytics.js
index e73aa103a1606..76c11e4ed636d 100644
--- a/ee/app/assets/javascripts/analytics/analytics_dashboards/data_sources/cube_analytics.js
+++ b/ee/app/assets/javascripts/analytics/analytics_dashboards/data_sources/cube_analytics.js
@@ -2,18 +2,19 @@ import { CubejsApi, HttpTransport } from '@cubejs-client/core';
import { convertToSnakeCase } from '~/lib/utils/text_utility';
import { pikadayToString } from '~/lib/utils/datetime_utility';
import csrf from '~/lib/utils/csrf';
+import {
+ EVENTS_TABLE_NAME,
+ SESSIONS_TABLE_NAME,
+} from 'ee/analytics/analytics_dashboards/constants';
// This can be any value because the cube proxy adds the real API token.
const CUBE_API_TOKEN = '1';
const PRODUCT_ANALYTICS_CUBE_PROXY = '/api/v4/projects/:id/product_analytics/request';
-const DEFAULT_COUNT_KEY = 'TrackedEvents.count';
// Filter measurement types must be lowercase
export const DATE_RANGE_FILTER_DIMENSIONS = {
- sessions: 'Sessions.startAt',
- trackedevents: 'TrackedEvents.utcTime',
- snowplowtrackedevents: 'SnowplowTrackedEvents.derivedTstamp',
- snowplowsessions: 'SnowplowSessions.startAt',
+ snowplowtrackedevents: `${EVENTS_TABLE_NAME}.derivedTstamp`,
+ snowplowsessions: `${SESSIONS_TABLE_NAME}.startAt`,
};
const convertToCommonChartFormat = (resultSet) => {
@@ -68,11 +69,7 @@ const convertToSingleValue = (resultSet, query) => {
const [measure] = query?.measures ?? [];
const [row] = resultSet.rawData();
- if (!row) {
- return 0;
- }
-
- return row[measure ?? DEFAULT_COUNT_KEY] ?? Object.values(row)[0] ?? 0;
+ return row[measure] ?? 0;
};
const buildDateRangeFilter = (query, queryOverrides, { startDate, endDate }) => {
diff --git a/ee/app/assets/javascripts/product_analytics/onboarding/components/instrumentation_instructions.vue b/ee/app/assets/javascripts/product_analytics/onboarding/components/instrumentation_instructions.vue
index 06f100ef839dd..b2bb71f6c2e8a 100644
--- a/ee/app/assets/javascripts/product_analytics/onboarding/components/instrumentation_instructions.vue
+++ b/ee/app/assets/javascripts/product_analytics/onboarding/components/instrumentation_instructions.vue
@@ -31,16 +31,6 @@ export default {
required: true,
},
},
- computed: {
- instructions() {
- return {
- install: INSTALL_NPM_PACKAGE,
- import: IMPORT_NPM_PACKAGE,
- init: INIT_TRACKING,
- htmlSetup: HTML_SCRIPT_SETUP,
- };
- },
- },
i18n: {
sdkClientsTitle: s__('ProductAnalytics|SDK clients'),
sdkHost: s__('ProductAnalytics|SDK host'),
@@ -68,6 +58,10 @@ export default {
),
},
BROWSER_SDK_DOCS_URL,
+ INSTALL_NPM_PACKAGE,
+ IMPORT_NPM_PACKAGE,
+ INIT_TRACKING,
+ HTML_SCRIPT_SETUP,
};
</script>
@@ -80,13 +74,13 @@ export default {
<h5 class="gl-mb-5">{{ $options.i18n.jsModuleTitle }}</h5>
<strong class="gl-display-block gl-mb-3">{{ $options.i18n.addNpmPackage }}</strong>
- <pre class="gl-mb-5">{{ instructions.install }}</pre>
+ <pre class="gl-mb-5">{{ $options.INSTALL_NPM_PACKAGE }}</pre>
<strong class="gl-display-block gl-mt-5 gl-mb-3">{{
$options.i18n.importNpmPackage
}}</strong>
- <pre class="gl-mb-5">{{ instructions.import }}</pre>
+ <pre class="gl-mb-5">{{ $options.IMPORT_NPM_PACKAGE }}</pre>
<strong class="gl-display-block gl-mt-5 gl-mb-3">{{ $options.i18n.initNpmPackage }}</strong>
- <pre class="gl-mb-5"><gl-sprintf :message="instructions.init">
+ <pre class="gl-mb-5"><gl-sprintf :message="$options.INIT_TRACKING">
<template #appId><span>{{ trackingKey }}</span></template>
<template #host><span>{{ collectorHost }}</span></template>
</gl-sprintf></pre>
@@ -97,7 +91,7 @@ export default {
<strong class="gl-display-block gl-mb-3">{{
$options.i18n.htmlScriptTagDescription
}}</strong>
- <pre class="gl-mb-5"><gl-sprintf :message="instructions.htmlSetup">
+ <pre class="gl-mb-5"><gl-sprintf :message="$options.HTML_SCRIPT_SETUP">
<template #appId><span>{{ trackingKey }}</span></template>
<template #host><span>{{ collectorHost }}</span></template>
</gl-sprintf></pre>
diff --git a/ee/spec/frontend/analytics/analytics_dashboards/components/data_sources/cube_analytics_spec.js b/ee/spec/frontend/analytics/analytics_dashboards/components/data_sources/cube_analytics_spec.js
index 6e054ba04c242..f2882d71d7fd4 100644
--- a/ee/spec/frontend/analytics/analytics_dashboards/components/data_sources/cube_analytics_spec.js
+++ b/ee/spec/frontend/analytics/analytics_dashboards/components/data_sources/cube_analytics_spec.js
@@ -1,7 +1,12 @@
import { CubejsApi, HttpTransport, __setMockLoad } from '@cubejs-client/core';
import { fetch } from 'ee/analytics/analytics_dashboards/data_sources/cube_analytics';
import { pikadayToString } from '~/lib/utils/datetime_utility';
-import { mockResultSet, mockFilters, mockTableWithLinksResultSet } from '../../mock_data';
+import {
+ mockResultSet,
+ mockFilters,
+ mockTableWithLinksResultSet,
+ mockResultSetWithNullValues,
+} from '../../mock_data';
const mockLoad = jest.fn().mockImplementation(() => mockResultSet);
@@ -48,97 +53,103 @@ describe('Cube Analytics Data Source', () => {
});
describe('formats the data', () => {
- it('returns the expected data format for line charts', async () => {
- const result = await fetch({ projectId, visualizationType, query });
-
- expect(result[0]).toMatchObject({
- data: [
- ['2022-11-09T00:00:00.000', 55],
- ['2022-11-10T00:00:00.000', 14],
- ],
- name: 'pageview, SnowplowTrackedEvents Count',
+ describe('charts', () => {
+ it('returns the expected data format for line charts', async () => {
+ const result = await fetch({ projectId, visualizationType, query });
+
+ expect(result[0]).toMatchObject({
+ data: [
+ ['2022-11-09T00:00:00.000', 55],
+ ['2022-11-10T00:00:00.000', 14],
+ ],
+ name: 'pageview, SnowplowTrackedEvents Count',
+ });
});
- });
- it('returns the expected data format for column charts', async () => {
- const result = await fetch({ projectId, visualizationType: 'ColumnChart', query });
+ it('returns the expected data format for column charts', async () => {
+ const result = await fetch({ projectId, visualizationType: 'ColumnChart', query });
- expect(result[0]).toMatchObject({
- data: [
- ['2022-11-09T00:00:00.000', 55],
- ['2022-11-10T00:00:00.000', 14],
- ],
- name: 'pageview, SnowplowTrackedEvents Count',
+ expect(result[0]).toMatchObject({
+ data: [
+ ['2022-11-09T00:00:00.000', 55],
+ ['2022-11-10T00:00:00.000', 14],
+ ],
+ name: 'pageview, SnowplowTrackedEvents Count',
+ });
});
});
- it('returns the expected data format for data tables', async () => {
- const result = await fetch({ projectId, visualizationType: 'DataTable', query });
+ describe('data tables', () => {
+ it('returns the expected data format for', async () => {
+ const result = await fetch({ projectId, visualizationType: 'DataTable', query });
- expect(result[0]).toMatchObject({
- count: '55',
- event_type: 'pageview',
- utc_time: '2022-11-09T00:00:00.000',
+ expect(result[0]).toMatchObject({
+ count: '55',
+ event_type: 'pageview',
+ utc_time: '2022-11-09T00:00:00.000',
+ });
});
- });
-
- it('returns the expected data format for data tables when links config is defined', async () => {
- mockLoad.mockImplementationOnce(() => mockTableWithLinksResultSet);
- const result = await fetch({
- projectId,
- visualizationType: 'DataTable',
- query: {
- measures: ['SnowplowTrackedEvents.pageViewsCount'],
- dimensions: ['SnowplowTrackedEvents.docPath', 'SnowplowTrackedEvents.url'],
- },
- visualizationOptions: {
- links: [
- {
- text: 'SnowplowTrackedEvents.docPath',
- href: 'SnowplowTrackedEvents.url',
- },
- ],
- },
- });
-
- expect(result[0]).toMatchObject({
- page_views_count: '1',
- doc_path: {
- text: '/foo',
- href: 'https://example.com/foo',
- },
+ it('returns the expected data format when links config is defined', async () => {
+ mockLoad.mockImplementationOnce(() => mockTableWithLinksResultSet);
+
+ const result = await fetch({
+ projectId,
+ visualizationType: 'DataTable',
+ query: {
+ measures: ['SnowplowTrackedEvents.pageViewsCount'],
+ dimensions: ['SnowplowTrackedEvents.docPath', 'SnowplowTrackedEvents.url'],
+ },
+ visualizationOptions: {
+ links: [
+ {
+ text: 'SnowplowTrackedEvents.docPath',
+ href: 'SnowplowTrackedEvents.url',
+ },
+ ],
+ },
+ });
+
+ expect(result[0]).toMatchObject({
+ page_views_count: '1',
+ doc_path: {
+ text: '/foo',
+ href: 'https://example.com/foo',
+ },
+ });
});
});
- it('returns the expected data format for single stats', async () => {
- const result = await fetch({ projectId, visualizationType: 'SingleStat', query });
+ describe('single stats', () => {
+ it('returns the expected data format', async () => {
+ const result = await fetch({ projectId, visualizationType: 'SingleStat', query });
- expect(result).toBe('36');
- });
-
- it('returns the expected data format for single stats with custom measure', async () => {
- const override = { measures: ['SnowplowTrackedEvents.url'] };
- const result = await fetch({
- projectId,
- visualizationType: 'SingleStat',
- query,
- queryOverrides: override,
+ expect(result).toBe('36');
});
- expect(result).toBe('https://example.com/us');
- });
+ it('returns the expected data format with custom measure', async () => {
+ const override = { measures: ['SnowplowTrackedEvents.url'] };
+ const result = await fetch({
+ projectId,
+ visualizationType: 'SingleStat',
+ query,
+ queryOverrides: override,
+ });
- it('returns the expected data format for single stats when the measure is unknown', async () => {
- const override = { measures: ['unknown'] };
- const result = await fetch({
- projectId,
- visualizationType: 'SingleStat',
- query,
- queryOverrides: override,
+ expect(result).toBe('https://example.com/us');
});
- expect(result).toBe('en-US');
+ it('returns 0 when the measure is null', async () => {
+ mockLoad.mockImplementationOnce(() => mockResultSetWithNullValues);
+
+ const result = await fetch({
+ projectId,
+ visualizationType: 'SingleStat',
+ query,
+ });
+
+ expect(result).toBe(0);
+ });
});
});
});
diff --git a/ee/spec/frontend/analytics/analytics_dashboards/mock_data.js b/ee/spec/frontend/analytics/analytics_dashboards/mock_data.js
index 6096578aa65e2..ce5c93ef358fe 100644
--- a/ee/spec/frontend/analytics/analytics_dashboards/mock_data.js
+++ b/ee/spec/frontend/analytics/analytics_dashboards/mock_data.js
@@ -306,6 +306,16 @@ export const mockTableWithLinksResultSet = {
],
};
+export const mockResultSetWithNullValues = {
+ rawData: () => [
+ {
+ 'SnowplowTrackedEvents.userLanguage': null,
+ 'SnowplowTrackedEvents.count': null,
+ 'SnowplowTrackedEvents.url': null,
+ },
+ ],
+};
+
export const mockFilters = {
startDate: new Date('2015-01-01'),
endDate: new Date('2016-01-01'),
--
GitLab