Convert messages to struct logs #6064
Conversation
emmyoop
changed the base branch from
main
to
ct-1047-proto_structured_logging
|
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. |
| import os | ||
| from typing import List | ||
| from dbt.constants import SECRET_ENV_PREFIX | ||
|
|
||
|
|
||
| def env_secrets() -> List[str]: | ||
| return [v for k, v in os.environ.items() if k.startswith(SECRET_ENV_PREFIX) and v.strip()] | ||
|
|
||
|
|
||
| def scrub_secrets(msg: str, secrets: List[str]) -> str: | ||
| scrubbed = msg | ||
|
|
||
| for secret in secrets: | ||
| scrubbed = scrubbed.replace(secret, "*****") | ||
|
|
||
| return scrubbed |
There was a problem hiding this comment.
I'm not sure I love what I did here. These were moved out because of some circular dependency issues. I'm not sure they even belong in the /events dir since they're used in various places.
There was a problem hiding this comment.
Yeah, but I can't think of a great place either. If a superior location emerges, we can move it.
|
Happy to wait on this until after the struct logging changes are merged in main - can change the target for the PR. |
emmyoop
requested review from
peterallenwebb
and removed request for
a team,
nathaniel-may and
ChenyuLInx
emmyoop
force-pushed
the
er/ct-1267-convert-msg
branch
from
46511cb to
d6c348e
Compare
There was a problem hiding this comment.
This is a really good start. But I'd like us to handle the "warn_or_error" calls somewhat differently, and convert as many of the "GeneralWarningMsg" instances to specific events instead. So warn_or_error might look something like:
def warn_or_error(event, node):
if flags.WARN_ERROR:
from dbt.exceptions import raise_compiler_error
raise_compiler_error(scrub_secrets(event.info.msg, env_secrets()), node)
else:
fire_event(event)
We're also going to be doing some work on handling and logging exceptions better too, so that "raise_compiler_error" is also likely to see some changes.
One thing people who consume our logs is asking for is being able to categorize the log events and exceptions, and that GeneralWarningMsg does not help.
| import os | ||
| from typing import List | ||
| from dbt.constants import SECRET_ENV_PREFIX | ||
|
|
||
|
|
||
| def env_secrets() -> List[str]: | ||
| return [v for k, v in os.environ.items() if k.startswith(SECRET_ENV_PREFIX) and v.strip()] | ||
|
|
||
|
|
||
| def scrub_secrets(msg: str, secrets: List[str]) -> str: | ||
| scrubbed = msg | ||
|
|
||
| for secret in secrets: | ||
| scrubbed = scrubbed.replace(secret, "*****") | ||
|
|
||
| return scrubbed |
There was a problem hiding this comment.
Yeah, but I can't think of a great place either. If a superior location emerges, we can move it.
| @@ -1327,7 +1327,7 @@ def catch_as_completed( | |||
| elif isinstance(exc, KeyboardInterrupt) or not isinstance(exc, Exception): | |||
| raise exc | |||
| else: | |||
| warn_or_error(f"Encountered an error while generating catalog: {str(exc)}") | |||
| warn_or_error(CatalogGenerationError(exc=str(exc))) | |||
There was a problem hiding this comment.
Nit: this paradigm of stringifying arguments to event types now has the option of adding this class as a subclass to the event class definition.
There was a problem hiding this comment.
That class is super cool. However, it is causing some issues because protobuf requires us to define the type of each field being passed in in a very narrow scope (types cannot be classes ie Exception). So exc comes in as an Exception and then does become a string in the message but that's not enough since proto expects the field itself to be a string already.
|
Close and reopen to try to trigger tests to run. |
resolves #5957
Description
Convert msg to be built in struct logs
Checklist
changie newto create a changelog entry