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

Fix static parser tracking logic #4332

Merged
merged 2 commits into from
Nov 24, 2021
Merged

Fix static parser tracking logic #4332

merged 2 commits into from
Nov 24, 2021

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Nov 24, 2021

Background

I noticed a few weird things this morning:

  • We have way too many cannot_parse results in anonymous usage tracking for v1 — many more than I'd expect if we're only sampling one out of every 50k models
  • Using latest v1, dbt --no-partial-parse parse ("cold parse") in our internal analytics project is taking 15-16 seconds, instead of 7-8 seconds. Indeed, that extra time is spent in tracking:

Screenshot 2021-11-24 at 12 23 28

Fix

We're firing a tracking event every single time the stable parser cannot_parse (or has_banned_macro). Instead, we should only populate the tracking result if we're sampling. When I make this change, a "cold parse" of our internal analytics project takes 7-8 seconds again. Hooray!

While here:

  • Consolidated the logic, so this check + result population happens as part of the "fall back to jinja" conditional branch
  • We should be checking experimental_sample, too, and firing a differently coded event depending (0xvs. 8x)

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@cla-bot cla-bot bot added the cla:yes label Nov 24, 2021
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

Oof. good catch! thanks for fixing this!

@jtcohen6 jtcohen6 merged commit 7f2d3cd into main Nov 24, 2021
@jtcohen6 jtcohen6 deleted the fix/static-parser-tracking branch November 24, 2021 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants