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

Replace tracking events: project_id, adapter_info #7231

Merged

Conversation

iknox-fa
Copy link
Contributor

@iknox-fa iknox-fa commented Mar 28, 2023

Resolves #6097 resolves #6098

Description

Replaces the project id, adapter_type, and adapter_unique_id events fields removed during the Click refactor due to timing issues. These values will now be generated in the init of the BaseTask since it's the earliest place we can get that info reliably.

Checklist

@iknox-fa iknox-fa requested a review from a team March 28, 2023 00:37
@iknox-fa iknox-fa requested a review from a team as a code owner March 28, 2023 00:37
@cla-bot cla-bot bot added the cla:yes label Mar 28, 2023
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@iknox-fa iknox-fa requested review from jtcohen6 and removed request for nathaniel-may March 28, 2023 00:39
INVOCATION_ENV_SPEC = "iglu:com.dbt/invocation_env/jsonschema/1-0-0"
PACKAGE_INSTALL_SPEC = "iglu:com.dbt/package_install/jsonschema/1-0-0"
RPC_REQUEST_SPEC = "iglu:com.dbt/rpc_request/jsonschema/1-0-1"
ADAPTER_INFO_SPEC = "iglu:com.dbt/adapter_info/jsonschema/1-0-0"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you haven't already, I think we'll need to add these two new validation schemas to https:/dbt-labs/iglu.sinter-collect.com

Doc: https://www.notion.so/dbtlabs/Add-new-metrics-to-tracking-in-dbt-3484cc31b8f449dfa2e960d2bbb15afe

Copy link
Contributor Author

@iknox-fa iknox-fa Mar 28, 2023

Choose a reason for hiding this comment

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

Yeppers.
https:/dbt-labs/iglu.sinter-collect.com/pull/67 although there seems to be an issue with circle CI we'll need to fix if we want to enforce the checks there.

Comment on lines +38 to +41
INVOCATION_ENV_SPEC = "iglu:com.dbt/invocation_env/jsonschema/1-0-0"
INVOCATION_SPEC = "iglu:com.dbt/invocation/jsonschema/1-0-2"
LOAD_ALL_TIMING_SPEC = "iglu:com.dbt/load_all_timing/jsonschema/1-0-3"
PACKAGE_INSTALL_SPEC = "iglu:com.dbt/package_install/jsonschema/1-0-0"
Copy link
Contributor

Choose a reason for hiding this comment

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

alphabetical order, nice 👍

Comment on lines +1 to +3
The events outlined here exist to support "very very old versions of dbt-core, which expected to look directly at the HEAD branch of this github repo to find validation schemas".

Eventually these should go away (see https:/dbt-labs/dbt-core/issues/7228)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@iknox-fa iknox-fa changed the title replaced tracking events removed in click work Replace tracking events: project_id, adapter_type, adapter_unique_id Mar 28, 2023
@iknox-fa iknox-fa changed the title Replace tracking events: project_id, adapter_type, adapter_unique_id Replace tracking events: project_id, adapter_info Mar 28, 2023
Comment on lines +96 to +102
dbt.tracking.track_project_id({"project_id": project_id})
dbt.tracking.track_adapter_info(
{
"adapter_type": adapter_type,
"adapter_unique_id": adapter_unique_id,
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

If project_id, adapter_type, and adapter_unique_id can be None, would we still want to fire these tracking events with only Nones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume so, but that's just mimicking the logic that was in place previously. @jtcohen6, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed!

Comment on lines +96 to +102
dbt.tracking.track_project_id({"project_id": project_id})
dbt.tracking.track_adapter_info(
{
"adapter_type": adapter_type,
"adapter_unique_id": adapter_unique_id,
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

agreed!

core/dbt/task/base.py Show resolved Hide resolved
@iknox-fa iknox-fa requested review from stu-k and jtcohen6 March 28, 2023 21:55
@iknox-fa iknox-fa merged commit 4741434 into main Mar 29, 2023
@iknox-fa iknox-fa deleted the iknox/CT-1368-1368-GH-6097-6098-replace-tracking-events branch March 29, 2023 14:41
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.

[CT-1368] New tracking event: Project ID [CT-1367] New tracking event: Adapter info
3 participants