-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add analytics event for extract column in chill mode #41774
Add analytics event for extract column in chill mode #41774
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.
structEvents
are deprecated google analytics events that should not be used. Please use snowplow events.
4c589ae
to
a9de2ad
Compare
}; | ||
|
||
function expressionsUsedBy(tag: string) { | ||
switch (tag) { |
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 is a bit fragile: when we add more extractions, we need to add the expressions used here too.
But AFAIC there is no easy way to get the expressions introduced by an extraction from the extraction itself.
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.
We could make ColumnExtractionInfo.tag
to be a enum and not just a string. We could also define a mapping type used here in a way to make it fail if there is a new tag that is not supported here. Like:
Record<ColumnExtractionTag, string>
Another way could to be to make analytic events match the expression tag either by updating the tag or the analytics spec.
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.
Ok I've done exactly that, please have another look.
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.
Inline comments.
@romeovs same as #42095 (comment) |
This gets the extractions from a column-extract drill, which is needed in the UI.
@ranquild please have another look, Braden added a helper so we could actually get the extractions from the drill. That is used to get the functions from the expression generated by the extraction correctly. |
720eb8b
to
b833aad
Compare
if (!arg || !(typeof arg === "object")) { | ||
return; | ||
} | ||
if ("operator" in arg) { |
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.
if (
typeof arg === "object") walk(...)` without the first if
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.
Hmm there are some type errors when I try that (because arg
can be an opaque object. Is there a case my code is missing?
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.
metabase/frontend/src/metabase-lib/filter.ts
Line 570 in 4b71e9a
function isExpression(arg: unknown): arg is ExpressionParts { |
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.
because typeof null === "object"
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.
feel free to move this utility to a different place (expression.ts?)
025f73e
to
f66be3a
Compare
Closes #41773
Description
Adds an analytics event that fires when the custom column is created from the table mode via Extract button in the column header