-
Notifications
You must be signed in to change notification settings - Fork 115
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
feat: add telemetry with mixpanel sdk #97
Conversation
manifest: Dict[str, Any], | ||
) -> NoReturn: | ||
if not self.disable_tracking: | ||
dbt_metadata = manifest.get("metadata") |
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.
Change dbt_metadata to the following:
manifest.get("metadata", {})
If metadata does not exist, it will return an empty dictionary instead of None
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.
I believe the mypy typing errors are on manifest, not the result of the get. Here's one of the errors I was getting originally:
pre_commit_dbt/tracking.py:35: error: Item "None" of "Optional[Any]" has no attribute "get"
Found 1 error in 1 file (checked 98 source files)
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.
Thats odd because there is a try except
logic in the hooks to return an error if the manifest
file is not detected.
Example:
try:
manifest = get_json(args.manifest)
except JsonOpenError as e:
print(f"Unable to load manifest file ({e})")
return 1
if not self.disable_tracking: | ||
dbt_metadata = manifest.get("metadata") | ||
distinct_id = ( | ||
dbt_metadata.get("user_id") if dbt_metadata is not None else None |
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.
Change distinct_id to the following:
distinct_id = dbt_metadata.get("user_id")
If user_id does not exist in dbt_metadata
, it will return None
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.
See comment above about typing and mypy
PR looks good to me, despite seeing a lot of repeated code now on hooks that hides the real purpose of the hook. A python decorator would work for that I think, what do you think? |
Yeah I think with some reworking a decorator could be a good option. There are a few track call properties that are being derived that might be a bit tricky, but I think we could either make some compromises (e.g. start time and end time can just be defined in the decorator instead of within the main function) or make some changes to the functions themselves or both. To me, this would help make maintaining the project easier, but the functions themselves are not surfacing enough of the event_props to make a decorator easy to implement. I'll ask around within Montreal Analytics to see if we can tap someone to take on this challenge, but if not are we ok with the added wait? @ssassi it seems that you're ok with this but would prefer a decorator, is that right? |
Sure, it's a "nice to have" implementation, but not 100% necessary. You could leave a "# TODO" comment in the meantime. |
This is a port of Dang's PR from dbt-gloss for adding Mixpanel telemetry.
This is a major update to add telemetry with Mixpanel to most of the hooks in the project (notably, the purely dbt hooks (e.g. dbt run, dbt docs generate) were excluded as these are entirely external to this project). As a result, we should make sure to get some thorough validation/UAT work done before merging this.
In addition to reviewing the changes, please clone this branch and try working with it with an example dbt project to give some more real world feedback as well. We've done testing on the original dbt-gloss PR, but it's worth double checking here too, of course!