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

Snowplow event for extract action via notebook shortcut #42095

Merged
merged 5 commits into from May 3, 2024

Conversation

romeovs
Copy link
Contributor

@romeovs romeovs commented May 1, 2024

Closes #42093

Depends on code from, #41774, so review that first.

Description

Adds an analytics event that fires when the custom column is created from the expression editor in the notebook via the extract shortcut.

To reproduce

  1. New question -> Sample data -> Orders
  2. Add new column
  3. Select Extract column shortcut
  4. pick an extraction
  5. Verify the snowplow event was triggered

@romeovs romeovs self-assigned this May 1, 2024
@romeovs romeovs requested a review from a team May 1, 2024 10:32
@romeovs romeovs changed the title 42093 extract analytics Snowplow event for extract action via notebook shortcut May 1, 2024
@romeovs romeovs added the no-backport Do not backport this PR to any branch label May 1, 2024
Copy link

replay-io bot commented May 1, 2024

Status Complete ↗︎
Commit 06a682e
Results
⚠️ 7 Flaky
2454 Passed

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.

@romeovs this looks wrong. Why? Because we have an abstraction (extraction tag) and then we need a mapping to the actual expression name. It means that the abstraction is leaky and doesn't work. Furthermore, it creates more issues.

If we need the actual expression name, let's change the tag to be the name of the underlying expression. @bshepherdson please take a look at this.

@ranquild ranquild requested a review from bshepherdson May 1, 2024 12:17
@bshepherdson
Copy link
Contributor

@romeovs this looks wrong. Why? Because we have an abstraction (extraction tag) and then we need a mapping to the actual expression name. It means that the abstraction is leaky and doesn't work. Furthermore, it creates more issues.

If we need the actual expression name, let's change the tag to be the name of the underlying expression. @bshepherdson please take a look at this.

It's not the case that an extraction corresponds 1-1 with an expression function. Some of them are nested - indeed the current extractions for Day of week, Month of year and Quarter of year expand to eg. [:month-name {} [:month {} column]].

That said, I think the table in extractions.ts is the wrong approach. If we want to list all the functions used by the extraction, then I think the right answer is to call Lib.expressionParts(Lib.extractionExpression(theExtraction)) and walk that tree to get all the operators.

@romeovs romeovs requested review from a team and ranquild May 3, 2024 10:40
@romeovs romeovs merged commit 021c35a into master May 3, 2024
112 checks passed
@romeovs romeovs deleted the 42093-extract-analytics branch May 3, 2024 11:40
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
Development

Successfully merging this pull request may close these issues.

Add analytics events for extract via column header
4 participants