Skip to content
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

Merged

Conversation

romeovs
Copy link
Contributor

@romeovs romeovs commented Apr 24, 2024

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

@romeovs romeovs requested a review from a team April 24, 2024 12:58
@romeovs romeovs self-assigned this Apr 24, 2024
Copy link

replay-io bot commented Apr 24, 2024

Status Complete ↗︎
Commit e9df66b
Results
⚠️ 6 Flaky
2447 Passed

@romeovs romeovs added the no-backport Do not backport this PR to any branch label Apr 24, 2024
Copy link
Contributor

@ranquild ranquild left a 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.

@romeovs romeovs force-pushed the 41773-add-analytics-event-to-chill-mode-extract-column branch from 4c589ae to a9de2ad Compare April 30, 2024 12:40
};

function expressionsUsedBy(tag: string) {
switch (tag) {
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@romeovs romeovs requested review from ranquild and a team April 30, 2024 12:45
Copy link
Contributor

@ranquild ranquild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline comments.

@ranquild
Copy link
Contributor

ranquild commented May 1, 2024

@romeovs same as #42095 (comment)

This gets the extractions from a column-extract drill, which is needed
in the UI.
@romeovs romeovs requested a review from camsaul as a code owner May 1, 2024 15:23
@romeovs
Copy link
Contributor Author

romeovs commented May 1, 2024

@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.

@romeovs romeovs force-pushed the 41773-add-analytics-event-to-chill-mode-extract-column branch from 720eb8b to b833aad Compare May 2, 2024 07:51
if (!arg || !(typeof arg === "object")) {
return;
}
if ("operator" in arg) {
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function isExpression(arg: unknown): arg is ExpressionParts {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because typeof null === "object"

Copy link
Contributor

@ranquild ranquild May 2, 2024

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?)

@romeovs romeovs linked an issue May 2, 2024 that may be closed by this pull request
@romeovs romeovs force-pushed the 41773-add-analytics-event-to-chill-mode-extract-column branch from 025f73e to f66be3a Compare May 2, 2024 17:37
@romeovs romeovs merged commit 50ef456 into master May 2, 2024
111 checks passed
@romeovs romeovs deleted the 41773-add-analytics-event-to-chill-mode-extract-column branch May 2, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
3 participants