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

CT 1537 fix event test and rename a couple of fields #6293

Merged
merged 6 commits into from
Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 6 additions & 6 deletions core/dbt/adapters/base/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@
from dbt.events.types import (
NewConnection,
ConnectionReused,
ConnectionLeftOpenInCleanup,
ConnectionLeftOpen,
ConnectionLeftOpen2,
ConnectionClosedInCleanup,
ConnectionClosed,
ConnectionClosed2,
Rollback,
RollbackFailed,
)
Expand Down Expand Up @@ -306,9 +306,9 @@ def cleanup_all(self) -> None:
with self.lock:
for connection in self.thread_connections.values():
if connection.state not in {"closed", "init"}:
fire_event(ConnectionLeftOpen(conn_name=cast_to_str(connection.name)))
fire_event(ConnectionLeftOpenInCleanup(conn_name=cast_to_str(connection.name)))
else:
fire_event(ConnectionClosed(conn_name=cast_to_str(connection.name)))
fire_event(ConnectionClosedInCleanup(conn_name=cast_to_str(connection.name)))
self.close(connection)

# garbage collect these connections
Expand Down Expand Up @@ -345,10 +345,10 @@ def _close_handle(cls, connection: Connection) -> None:
"""Perform the actual close operation."""
# On windows, sometimes connection handles don't have a close() attr.
if hasattr(connection.handle, "close"):
fire_event(ConnectionClosed2(conn_name=cast_to_str(connection.name)))
fire_event(ConnectionClosed(conn_name=cast_to_str(connection.name)))
connection.handle.close()
else:
fire_event(ConnectionLeftOpen2(conn_name=cast_to_str(connection.name)))
fire_event(ConnectionLeftOpen(conn_name=cast_to_str(connection.name)))

@classmethod
def _rollback(cls, connection: Connection) -> None:
Expand Down
6 changes: 3 additions & 3 deletions core/dbt/context/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
disallow_secret_env_var,
)
from dbt.events.functions import fire_event, get_invocation_id
from dbt.events.types import MacroEventInfo, MacroEventDebug
from dbt.events.types import JinjaLogInfo, JinjaLogDebug
Copy link
Member

@emmyoop emmyoop Nov 18, 2022

Choose a reason for hiding this comment

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

Along these same lines there's also a GeneralMacroWarning that is the catchall for warn defined in exceptions.py. It's worth renaming that along with these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't look like it's actually used anywhere. Can we just remove it?

Copy link
Member

Choose a reason for hiding this comment

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

It's used within macros (docs)

Copy link
Contributor Author

@gshank gshank Nov 18, 2022

Choose a reason for hiding this comment

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

So it's exported into the context. So should we call it JinjaLogWarning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or JinjaLogWarn?

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 went with JinjaLogWarning.

from dbt.version import __version__ as dbt_version

# These modules are added to the context. Consider alternative
Expand Down Expand Up @@ -557,9 +557,9 @@ def log(msg: str, info: bool = False) -> str:
{% endmacro %}"
"""
if info:
fire_event(MacroEventInfo(msg=msg))
fire_event(JinjaLogInfo(msg=msg))
else:
fire_event(MacroEventDebug(msg=msg))
fire_event(JinjaLogDebug(msg=msg))
return ""

@contextproperty
Expand Down
7 changes: 4 additions & 3 deletions core/dbt/events/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ The event module provides types that represent what is happening in dbt in `even
When events are processed via `fire_event`, nearly everything is logged. Whether or not the user has enabled the debug flag, all debug messages are still logged to the file. However, some events are particularly time consuming to construct because they return a huge amount of data. Today, the only messages in this category are cache events and are only logged if the `--log-cache-events` flag is on. This is important because these messages should not be created unless they are going to be logged, because they cause a noticable performance degredation. These events use a "fire_event_if" functions.

# Adding a New Event
New events need to have a proto message definition created in core/dbt/events/types.proto. Every message must include EventInfo as the first field, named "info" and numbered 1. To update the proto_types.py file, in the core/dbt/events directory: ```protoc --python_betterproto_out . types.proto```

A matching class needs to be created in the core/dbt/events/types.py file, which will have two superclasses, the "Level" mixin and the generated class from proto_types.py. These classes will also generally have two methods, a "code" method that returns the event code, and a "message" method that is used to construct the "msg" from the event fields. In addition the "Level" mixin will provide a "level_tag" method to set the level (which can also be overridden using the "info" convenience function from functions.py)
* Add a new message in types.proto with an EventInfo field first
* run the protoc compiler to update proto_types.py: ```protoc --python_betterproto_out . types.proto```
* Add a wrapping class in core/dbt/event/types.py with a Level superclass and the superclass from proto_types.py, plus code and message methods
* Add the class to tests/unit/test_events.py

Note that no attributes can exist in these event classes except for fields defined in the protobuf definitions, because the betterproto metaclass will throw an error. Betterproto provides a to_dict() method to convert the generated classes to a dictionary and from that to json. However some attributes will successfully convert to dictionaries but not to serialized protobufs, so we need to test both output formats.

Expand Down
26 changes: 14 additions & 12 deletions core/dbt/events/proto_types.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 0 additions & 15 deletions core/dbt/events/test_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,18 +61,3 @@ def code(self):

def message(self) -> str:
return f"Unit Test: {self.msg}"


# since mypy doesn't run on every file we need to suggest to mypy that every
# class gets instantiated. But we don't actually want to run this code.
# making the conditional `if False` causes mypy to skip it as dead code so
# we need to skirt around that by computing something it doesn't check statically.
#
# TODO remove these lines once we run mypy everywhere.
if 1 == 0:
IntegrationTestInfo(msg="")
IntegrationTestDebug(msg="")
IntegrationTestWarn(msg="")
IntegrationTestError(msg="")
IntegrationTestException(msg="")
UnitTestInfo(msg="")
26 changes: 14 additions & 12 deletions core/dbt/events/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ message EventInfo {
string thread = 7;
google.protobuf.Timestamp ts = 8;
map<string, string> extra = 9;
string category = 10;
Copy link
Member

Choose a reason for hiding this comment

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

Is this eventually going to be the log category? Am I missing how this gets populated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, right now it's just a placeholder. Eventually we'll populate it somehow...

Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue so we don't lose the fact we still need to actually populate it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly it's your ticket #5958.

}

// TimingInfo
Expand Down Expand Up @@ -308,13 +309,13 @@ message ConnectionReused {
}

// E007
message ConnectionLeftOpen {
message ConnectionLeftOpenInCleanup {
EventInfo info = 1;
string conn_name = 2;
}

// E008
message ConnectionClosed {
message ConnectionClosedInCleanup {
EventInfo info = 1;
string conn_name = 2;
}
Expand All @@ -327,13 +328,13 @@ message RollbackFailed {
}

// E010
message ConnectionClosed2 {
message ConnectionClosed {
EventInfo info = 1;
string conn_name = 2;
}

// E011
message ConnectionLeftOpen2 {
message ConnectionLeftOpen {
EventInfo info = 1;
string conn_name = 2;
}
Expand Down Expand Up @@ -943,7 +944,7 @@ message NodeNotFoundOrDisabled {
}

// I061
message GeneralMacroWarning {
message JinjaLogWarning {
EventInfo info = 1;
string msg = 2;
}
Expand Down Expand Up @@ -1015,13 +1016,13 @@ message SelectorReportInvalidSelector {
}

// M011
message MacroEventInfo {
message JinjaLogInfo {
EventInfo info = 1;
string msg = 2;
}

// M012
message MacroEventDebug {
message JinjaLogDebug {
EventInfo info = 1;
string msg = 2;
}
Expand Down Expand Up @@ -1227,11 +1228,12 @@ message LogSeedResult {
EventInfo info = 1;
NodeInfo node_info = 2;
string status = 3;
int32 index = 4;
int32 total = 5;
float execution_time = 6;
string schema = 7;
string relation = 8;
string result_message = 4;
int32 index = 5;
int32 total = 6;
float execution_time = 7;
string schema = 8;
string relation = 9;
}

// Skipped Q017
Expand Down
Loading