-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Datanode migration telemetry #20225
Datanode migration telemetry #20225
Conversation
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.
Looks good in general, some small suggestions.
I haven't tested it yet, but as it is telemetry it should be good.
const sendTelemetry = useSendTelemetry(); | ||
|
||
const handleTabSwitch = (e) => { | ||
sendTelemetry((e?.target?.innerText === 'Upload CA') |
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.
Would be nice to move Upload CA
into a constant and reuse it at line 63 to avoid bugs.
sendTelemetry(TELEMETRY_EVENT_TYPE.DATANODE_MIGRATION.MIGRATION_TYPE_SELECTED, { | ||
app_pathname: 'datanode', | ||
app_section: 'migration', | ||
event_details: { migration_type: (step === 'SELECT_ROLLING_UPGRADE_MIGRATION') ? 'IN-PLACE' : 'REMOTE REINDEX' }, |
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.
Same here, constant for the step would be nice.
import { TELEMETRY_EVENT_TYPE } from 'logic/telemetry/Constants'; | ||
import type { TelemetryEventType } from 'logic/telemetry/TelemetryContext'; | ||
|
||
import useMigrationState from '../../hooks/useMigrationState'; |
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.
import useMigrationState from '../../hooks/useMigrationState'; | |
import useMigrationState from 'components/datanode/hooks/useMigrationState'; |
...eb-interface/src/components/datanode/migrations/common/MigrationStepTriggerButtonToolbar.tsx
Show resolved
Hide resolved
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.
Thank you for the changes, looks good!
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.
Tested and looks good!
Motivation and Context
fixes #20086
Types of changes
Checklist: