chore: improve types and documentation for telemetry #6176
Conversation
| @@ -1509,10 +1509,6 @@ export const connect = ( | |||
| track( | |||
| 'Connection Attempt', | |||
| { | |||
| is_favorite: connectionInfo.savedConnectionType === 'favorite', | |||
There was a problem hiding this comment.
drive by, these attributes aren't meaningful anymore with multiple connections
There was a problem hiding this comment.
Are they, though? We still have the concept of a favourite that we haven't removed. Can't ever remove it if we don't keep stats on whether it gets used.
There was a problem hiding this comment.
I suggested removing them because the significance of is_favorite is not the same as it was before. Earlier is_favorite was the only way to differentiate wether a connection is saved or recent but now all the connections are saved and is_favorite is more UI related, allowing us to group connections together. So for the telemetry point that it used to represent earlier, it does not make sense to keep it anymore. However if we would like to track connections being marked as favorite maybe we can introduce a different attribute?
There was a problem hiding this comment.
ahh ok, so is favorite is still a thing, then i will keep it
There was a problem hiding this comment.
and is recent doesn't make sense because everything saved now right?
There was a problem hiding this comment.
I think is_recent can be dropped, yes.
There was a problem hiding this comment.
ok re-added is_favorite then! thanks!
| @@ -489,7 +489,7 @@ class CrudStoreImpl | |||
| * to update if the labels change at some point. | |||
| */ | |||
| modeForTelemetry() { | |||
| return this.state.view.toLowerCase(); | |||
| return this.state.view.toLowerCase() as 'list' | 'json' | 'table'; | |||
There was a problem hiding this comment.
is there a better way?
There was a problem hiding this comment.
as Lowercase<DocumentView>?
| @@ -143,7 +143,7 @@ export const StageEditor = ({ | |||
| { | |||
| num_stages: num_stages, | |||
| stage_index: index + 1, | |||
| stage_action: 'stage_content_changed', | |||
| stage_action: 'stage_content_changed' as const, | |||
There was a problem hiding this comment.
I'm not understanding what these as const bits are doing and why they would be necessary. Why not give the whole object a type?
There was a problem hiding this comment.
as in move as const at the end? They are there so stage_action is not considered a string
There was a problem hiding this comment.
ah wait, you are right this should not be needed here though. I'll take another look and remove them where is redundant
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes