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.

Sign up for GitHub

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

Jump to bottom

Add adapter telemetry method for core model run snowplow event. #328

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+57 −0
Open

Add adapter telemetry method for core model run snowplow event. #328

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
793db57
Add adapter telemetry method for core model run snowplow event.
VersusFacit Oct 16, 2024
31a772e
lint.
VersusFacit Oct 16, 2024
9fcc789
Changelog.
VersusFacit Oct 16, 2024
ebc66b7
Merge branch 'main' into ADAP-301/add-adapter-telemetry
VersusFacit Oct 18, 2024
755f446
Float type up package for importers.
VersusFacit Oct 18, 2024
298e9d8
Revamp based on code review comments and add unit test.
VersusFacit Oct 22, 2024
112cbf3
Merge branch 'main' into ADAP-301/add-adapter-telemetry
VersusFacit Oct 22, 2024
b7cd418
Fix linter by altering test.
VersusFacit Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Under the Hood-20241016-033159.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Under the Hood
body: Add adapter telemetry.
time: 2024-10-16T03:31:59.334011-07:00
custom:
Author: versusfacit
Issue: "301"
1 change: 1 addition & 0 deletions dbt/adapters/base/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@
BaseRelation,
RelationType,
SchemaSearchMap,
AdapterTrackingRelationInfo,
)
12 changes: 12 additions & 0 deletions dbt/adapters/base/impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from contextlib import contextmanager
from datetime import datetime
from enum import Enum
from importlib import import_module
from multiprocessing.context import SpawnContext
from typing import (
Any,
Expand Down Expand Up @@ -61,12 +62,14 @@
ComponentName,
InformationSchema,
SchemaSearchMap,
AdapterTrackingRelationInfo,
)
from dbt.adapters.cache import RelationsCache, _make_ref_key_dict
from dbt.adapters.capability import Capability, CapabilityDict
from dbt.adapters.contracts.connection import Credentials
from dbt.adapters.contracts.macros import MacroResolverProtocol
from dbt.adapters.contracts.relation import RelationConfig

from dbt.adapters.events.types import (
CacheMiss,
CatalogGenerationError,
Expand Down Expand Up @@ -1743,6 +1746,15 @@ def capabilities(cls) -> CapabilityDict:
def supports(cls, capability: Capability) -> bool:
return bool(cls.capabilities()[capability])

@available
VersusFacit marked this conversation as resolved.
Show resolved Hide resolved
@classmethod
def get_adapter_run_info(cls, config: RelationConfig) -> AdapterTrackingRelationInfo:
Copy link
Contributor

Choose a reason for hiding this comment

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

RelationConfig doesn't feel like the correct type. I would have guessed this is some version of NodeConfig.

Copy link
Contributor Author

@VersusFacit VersusFacit Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have a good handle on the differences here. When I did introspection, we are passing in a RelationConfig. I bet I'm missing some appreciation of all these different Config objects and perhaps I was "lead on" by the suggestion for RelationConfig in the original ticket. If you feel strongly on this type, please do offer your thoughts here! I'm actively learning here.

return AdapterTrackingRelationInfo(
VersusFacit marked this conversation as resolved.
Show resolved Hide resolved
adapter_name="base_adapter",
version=import_module("dbt.adapters.__about__").version,
adapter_details={},
)


COLUMNS_EQUAL_SQL = """
with diff_count as (
Expand Down
7 changes: 7 additions & 0 deletions dbt/adapters/base/relation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,3 +531,10 @@ def flatten(self, allow_multiple_databases: bool = False) -> "SchemaSearchMap":
)

return new


@dataclass(frozen=True, eq=False, repr=False)
class AdapterTrackingRelationInfo(FakeAPIObject, Hashable):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting case to me: should this actually be considered a "contract" like AdapterResponse or is it tied to the concept of a relation (i.e. the thing it's tracking)? If the former we should probably put it in it's own module in dbt/adapters/contracts. I think there's an argument for both so curious what others think

Copy link
Contributor Author

@VersusFacit VersusFacit Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So just putting this forward -- I originally had it as a contract. Then it ran into "can't instantiate protocol" errors. So I'm not sure it should be a contract on that alone!

To me, I had it as something related to the object it's related to because I think in terms of object traits a lot :)

adapter_name: str
version: str
adapter_details: Any
Loading