-
Notifications
You must be signed in to change notification settings - Fork 393
RI-7218 Add telemetry collection for Create Vector Search Index wizard #4807
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
Changes from all commits
fe35ec3
7074c9b
a5d5f32
4e8a96b
68252b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,8 @@ export interface IBulkActionOverview { | |
databaseId: string; | ||
duration: number; | ||
type: BulkActionType; | ||
status: BulkActionStatus; | ||
filter: IBulkActionFilterOverview; | ||
status: BulkActionStatus; // Note: This can be null, according to the API response | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, we should update the interface itself, but I was not brave enough to do it at this point, afraid of breaking something unexpected. |
||
filter: IBulkActionFilterOverview; // Note: This can be null, according to the API response | ||
progress: IBulkActionProgressOverview; | ||
summary: IBulkActionSummaryOverview; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { Factory } from 'fishery' | ||
import { faker } from '@faker-js/faker' | ||
import { RedisDataType } from 'uiSrc/constants' | ||
import { IBulkActionOverview } from 'apiSrc/modules/bulk-actions/interfaces/bulk-action-overview.interface' | ||
import { IBulkActionFilterOverview } from 'apiSrc/modules/bulk-actions/interfaces/bulk-action-filter-overview.interface' | ||
import { IBulkActionProgressOverview } from 'apiSrc/modules/bulk-actions/interfaces/bulk-action-progress-overview.interface' | ||
import { IBulkActionSummaryOverview } from 'apiSrc/modules/bulk-actions/interfaces/bulk-action-summary-overview.interface' | ||
import { | ||
BulkActionStatus, | ||
BulkActionType, | ||
} from 'apiSrc/modules/bulk-actions/constants' | ||
|
||
export const bulkActionOverviewFactory = Factory.define<IBulkActionOverview>( | ||
({ sequence }) => ({ | ||
id: `bulk-action-${sequence}`, | ||
databaseId: faker.string.ulid(), | ||
type: faker.helpers.enumValue(BulkActionType), | ||
summary: bulkActionSummaryOverviewFactory.build(), | ||
progress: bulkActionProgressOverviewFactory.build(), | ||
filter: bulkActionFilterOverviewFactory.build(), | ||
status: faker.helpers.enumValue(BulkActionStatus), | ||
duration: faker.number.int({ min: 10, max: 100 }), | ||
}), | ||
) | ||
|
||
export const bulkActionSummaryOverviewFactory = | ||
Factory.define<IBulkActionSummaryOverview>(() => ({ | ||
processed: faker.number.int({ min: 200, max: 299 }), | ||
succeed: faker.number.int({ min: 300, max: 399 }), | ||
failed: faker.number.int({ min: 400, max: 499 }), | ||
errors: [], | ||
keys: [], | ||
})) | ||
|
||
export const bulkActionProgressOverviewFactory = | ||
Factory.define<IBulkActionProgressOverview>(() => ({ | ||
total: faker.number.int({ min: 100, max: 1000 }), | ||
scanned: faker.number.int({ min: 0, max: 1000 }), | ||
})) | ||
|
||
export const bulkActionFilterOverviewFactory = | ||
Factory.define<IBulkActionFilterOverview>(() => ({ | ||
type: faker.helpers.enumValue(RedisDataType), | ||
match: faker.string.uuid(), | ||
})) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { Factory } from 'fishery' | ||
import { faker } from '@faker-js/faker' | ||
import { CommandExecutionStatus } from 'uiSrc/slices/interfaces/cli' | ||
|
||
// Note: Types are temporaryly not binded properly due to issues in the Jset setup | ||
// SyntaxError: redisinsight/api/src/modules/workbench/models/command-execution.ts: Support for the experimental syntax 'decorators' isn't currently enabled (32:3): | ||
valkirilov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// export const commandExecutionFactory = Factory.define<CommandExecution>( | ||
valkirilov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
export const commandExecutionFactory = Factory.define(({ sequence }) => ({ | ||
id: sequence.toString() ?? faker.string.uuid(), | ||
databaseId: faker.string.ulid(), | ||
db: faker.number.int({ min: 0, max: 15 }), | ||
type: faker.helpers.arrayElement(['WORKBENCH', 'SEARCH']), // faker.helpers.enumValue(CommandExecutionType), | ||
mode: faker.helpers.arrayElement(['RAW', 'ASCII']), // faker.helpers.enumValue(RunQueryMode), | ||
resultsMode: faker.helpers.arrayElement(['DEFAULT', 'GROUP_MODE', 'SILENT']), // faker.helpers.enumValue(ResultsMode), | ||
valkirilov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
command: faker.lorem.paragraph(), | ||
result: commandExecutionResultFactory.buildList(1), | ||
executionTime: faker.number.int({ min: 1000, max: 5000 }), | ||
createdAt: faker.date.past(), | ||
})) | ||
|
||
// export const commandExecutionResultFactory = Factory.define<CommandExecutionResult>(() => { | ||
export const commandExecutionResultFactory = Factory.define(() => { | ||
const includeSizeLimitExceeded = faker.datatype.boolean() | ||
|
||
return { | ||
status: faker.helpers.enumValue(CommandExecutionStatus), | ||
response: faker.lorem.paragraph(), | ||
|
||
// Optional properties | ||
...(includeSizeLimitExceeded && { | ||
sizeLimitExceeded: faker.datatype.boolean(), | ||
}), | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { rest, RestHandler } from 'msw' | ||
import { ApiEndpoints } from 'uiSrc/constants' | ||
import { getMswURL } from 'uiSrc/utils/test-utils' | ||
import { getUrl } from 'uiSrc/utils' | ||
import { IBulkActionOverview } from 'uiSrc/slices/interfaces' | ||
import { bulkActionOverviewFactory } from 'uiSrc/mocks/factories/browser/bulkActions/bulkActionOverview.factory' | ||
import { INSTANCE_ID_MOCK } from '../instances/instancesHandlers' | ||
|
||
const handlers: RestHandler[] = [ | ||
rest.post<IBulkActionOverview>( | ||
getMswURL( | ||
getUrl( | ||
INSTANCE_ID_MOCK, | ||
ApiEndpoints.BULK_ACTIONS_IMPORT_VECTOR_COLLECTION, | ||
), | ||
), | ||
async (_req, res, ctx) => | ||
res(ctx.status(200), ctx.json(bulkActionOverviewFactory.build())), | ||
), | ||
] | ||
|
||
export default handlers |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
import { DefaultBodyType, MockedRequest, RestHandler } from 'msw' | ||
|
||
import redisearch from './redisearchHandlers' | ||
import bulkActions from './bulkActionsHandlers' | ||
|
||
const handlers: RestHandler<MockedRequest<DefaultBodyType>>[] = [].concat( | ||
redisearch, | ||
bulkActions, | ||
) | ||
export default handlers |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { rest, RestHandler } from 'msw' | ||
import { ApiEndpoints } from 'uiSrc/constants' | ||
import { getMswURL } from 'uiSrc/utils/test-utils' | ||
import { getUrl } from 'uiSrc/utils' | ||
import { CommandExecution } from 'uiSrc/slices/interfaces' | ||
import { commandExecutionFactory } from 'uiSrc/mocks/factories/workbench/commandExectution.factory' | ||
import { INSTANCE_ID_MOCK } from '../instances/instancesHandlers' | ||
|
||
const handlers: RestHandler[] = [ | ||
rest.post<CommandExecution[]>( | ||
getMswURL( | ||
getUrl(INSTANCE_ID_MOCK, ApiEndpoints.WORKBENCH_COMMAND_EXECUTIONS), | ||
), | ||
async (_req, res, ctx) => | ||
res(ctx.status(200), ctx.json(commandExecutionFactory.buildList(1))), | ||
), | ||
] | ||
|
||
export default handlers |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { DefaultBodyType, MockedRequest, RestHandler } from 'msw' | ||
|
||
import commands from './commands' | ||
|
||
const handlers: RestHandler<MockedRequest<DefaultBodyType>>[] = [].concat( | ||
commands, | ||
) | ||
export default handlers |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
import React from 'react' | ||
import { | ||
render, | ||
screen, | ||
fireEvent, | ||
initialStateDefault, | ||
mockStore, | ||
} from 'uiSrc/utils/test-utils' | ||
import { RootState } from 'uiSrc/slices/store' | ||
import { sendEventTelemetry, TelemetryEvent } from 'uiSrc/telemetry' | ||
import { INSTANCE_ID_MOCK } from 'uiSrc/mocks/handlers/analytics/dbAnalysisHistoryHandlers' | ||
import { VectorSearchCreateIndex } from './VectorSearchCreateIndex' | ||
import { SampleDataContent, SampleDataType, SearchIndexType } from './types' | ||
|
||
// Mock the telemetry module, so we don't send actual telemetry data during tests | ||
jest.mock('uiSrc/telemetry', () => ({ | ||
...jest.requireActual('uiSrc/telemetry'), | ||
sendEventTelemetry: jest.fn(), | ||
})) | ||
|
||
const renderVectorSearchCreateIndexComponent = () => { | ||
const testState: RootState = { | ||
...initialStateDefault, | ||
connections: { | ||
...initialStateDefault.connections, | ||
instances: { | ||
...initialStateDefault.connections.instances, | ||
connectedInstance: { | ||
...initialStateDefault.connections.instances.connectedInstance, | ||
id: INSTANCE_ID_MOCK, | ||
name: 'test-instance', | ||
host: 'localhost', | ||
port: 6379, | ||
modules: [], | ||
}, | ||
}, | ||
}, | ||
} | ||
const store = mockStore(testState) | ||
|
||
return render(<VectorSearchCreateIndex />, { store }) | ||
} | ||
|
||
describe('VectorSearchCreateIndex', () => { | ||
beforeEach(() => { | ||
jest.clearAllMocks() | ||
}) | ||
|
||
it('should render correctly', () => { | ||
const { container } = renderVectorSearchCreateIndexComponent() | ||
|
||
expect(container).toBeInTheDocument() | ||
}) | ||
|
||
describe('Telemetry', () => { | ||
it('should send telemetry events on start step', () => { | ||
renderVectorSearchCreateIndexComponent() | ||
|
||
expect(sendEventTelemetry).toHaveBeenCalledTimes(1) | ||
expect(sendEventTelemetry).toHaveBeenCalledWith({ | ||
event: TelemetryEvent.VECTOR_SEARCH_ONBOARDING_TRIGGERED, | ||
eventData: { databaseId: INSTANCE_ID_MOCK }, | ||
}) | ||
}) | ||
|
||
it('should send telemetry events on index info step', () => { | ||
renderVectorSearchCreateIndexComponent() | ||
|
||
// Select the index type | ||
const indexTypeRadio = screen.getByText('Redis Query Engine') | ||
fireEvent.click(indexTypeRadio) | ||
|
||
// Select the sample dataset | ||
const sampleDataRadio = screen.getByText('Pre-set data') | ||
fireEvent.click(sampleDataRadio) | ||
|
||
// Select data content | ||
const dataContentRadio = screen.getByText('E-commerce Discovery') | ||
fireEvent.click(dataContentRadio) | ||
|
||
// Simulate going to the index info step | ||
const buttonNext = screen.getByText('Proceed to index') | ||
fireEvent.click(buttonNext) | ||
|
||
expect(sendEventTelemetry).toHaveBeenCalledTimes(2) | ||
expect(sendEventTelemetry).toHaveBeenNthCalledWith(2, { | ||
event: TelemetryEvent.VECTOR_SEARCH_ONBOARDING_PROCEED_TO_INDEX_INFO, | ||
eventData: { | ||
databaseId: INSTANCE_ID_MOCK, | ||
indexType: SearchIndexType.REDIS_QUERY_ENGINE, | ||
sampleDataType: SampleDataType.PRESET_DATA, | ||
dataContent: SampleDataContent.E_COMMERCE_DISCOVERY, | ||
}, | ||
}) | ||
}) | ||
|
||
it('should send telemetry events on create index step', () => { | ||
renderVectorSearchCreateIndexComponent() | ||
|
||
// Simulate going to the index info step | ||
const buttonNext = screen.getByText('Proceed to index') | ||
fireEvent.click(buttonNext) | ||
|
||
// Simulate creating the index | ||
const buttonCreateIndex = screen.getByText('Create index') | ||
fireEvent.click(buttonCreateIndex) | ||
|
||
expect(sendEventTelemetry).toHaveBeenCalledTimes(3) | ||
expect(sendEventTelemetry).toHaveBeenNthCalledWith(3, { | ||
event: TelemetryEvent.VECTOR_SEARCH_ONBOARDING_PROCEED_TO_QUERIES, | ||
eventData: { | ||
databaseId: INSTANCE_ID_MOCK, | ||
}, | ||
}) | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,12 @@ | ||
import React, { useState } from 'react' | ||
import React, { useEffect, useState } from 'react' | ||
import { useParams } from 'react-router-dom' | ||
|
||
import { Stepper } from '@redis-ui/components' | ||
import { Title } from 'uiSrc/components/base/text' | ||
import { Button, SecondaryButton } from 'uiSrc/components/base/forms/buttons' | ||
import { ChevronLeftIcon } from 'uiSrc/components/base/icons' | ||
|
||
import { sendEventTelemetry, TelemetryEvent } from 'uiSrc/telemetry' | ||
import { selectedBikesIndexFields, stepContents } from './steps' | ||
import { | ||
CreateSearchIndexParameters, | ||
|
@@ -59,6 +60,7 @@ | |
const isFinalStep = step === stepContents.length - 1 | ||
if (isFinalStep) { | ||
createIndex(createSearchIndexParameters) | ||
collectCreateIndexStepTelemetry() | ||
return | ||
} | ||
|
||
|
@@ -68,6 +70,57 @@ | |
setStep(step - 1) | ||
} | ||
|
||
useEffect(() => { | ||
collectTelemetry(step) | ||
}, [step]) | ||
|
||
const collectTelemetry = (step: number): void => { | ||
switch (step) { | ||
case 1: | ||
collectStartStepTelemetry() | ||
break | ||
case 2: | ||
collectIndexInfoStepTelemetry() | ||
break | ||
case 3: | ||
collectCreateIndexStepTelemetry() | ||
break | ||
Check warning on line 87 in redisinsight/ui/src/pages/vector-search/create-index/VectorSearchCreateIndex.tsx
|
||
Comment on lines
+73
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a big fan of this. It's not even code that's worth extracting to a custom hook (all the telemetry callbacks). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I wanted to do the same, but in order to extract them as a custom hook, we should prop drill all the state params (necessary for the telemetry to work), and at the same time, we don't have a shared context to do it in a clean way. This is the "best" I could do without additional refactoring of the main state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a hook but a method is what I suggested. Having these functions inside the component increase it's complexity unneededly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I made some refactoring in e554f1a to offload the current component, as you suggested. I moved it out to a hook (instead of just a function) so we can pass down less props (like Let me know if that was what you suggested 🙂 |
||
default: | ||
// No telemetry for other steps | ||
break | ||
} | ||
} | ||
|
||
const collectStartStepTelemetry = (): void => { | ||
sendEventTelemetry({ | ||
event: TelemetryEvent.VECTOR_SEARCH_ONBOARDING_TRIGGERED, | ||
eventData: { | ||
databaseId: instanceId, | ||
}, | ||
}) | ||
} | ||
|
||
const collectIndexInfoStepTelemetry = (): void => { | ||
sendEventTelemetry({ | ||
event: TelemetryEvent.VECTOR_SEARCH_ONBOARDING_PROCEED_TO_INDEX_INFO, | ||
eventData: { | ||
databaseId: instanceId, | ||
indexType: createSearchIndexParameters.searchIndexType, | ||
sampleDataType: createSearchIndexParameters.sampleDataType, | ||
dataContent: createSearchIndexParameters.dataContent, | ||
}, | ||
}) | ||
} | ||
|
||
const collectCreateIndexStepTelemetry = (): void => { | ||
sendEventTelemetry({ | ||
event: TelemetryEvent.VECTOR_SEARCH_ONBOARDING_PROCEED_TO_QUERIES, | ||
eventData: { | ||
databaseId: instanceId, | ||
}, | ||
}) | ||
} | ||
|
||
if (success) { | ||
return <>Success!</> | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alias exist for the regular bundler, but was not available for Jest, causing the tests that rely on such imports to fail.