-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow custom metadata to be attached to every log #5999
Merged
emmyoop
merged 14 commits into
ct-1047-proto_structured_logging
from
er/ct-624-metadata-logs-struct-logging
Oct 13, 2022
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
24d5145
WIP
emmyoop 86b5371
use extra dict
emmyoop fe032a1
remove commented out code
emmyoop 4f4dc1f
fix duplicates
emmyoop 39b3945
add test
emmyoop c89ab43
tests wip
emmyoop 204872c
consolidate
emmyoop 768825b
working, but needs some cleanup
emmyoop c5ed871
convert back to globals
emmyoop b5eb622
Merge branch 'ct-1047-proto_structured_logging' into er/ct-624-metada…
emmyoop fa07270
reset globals in tests
emmyoop 9160b51
fix up some imports
emmyoop eb3770b
fix text for windows
emmyoop d619672
modify test for windows
emmyoop File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
SECRET_ENV_PREFIX = "DBT_ENV_SECRET_" | ||
DEFAULT_ENV_PLACEHOLDER = "DBT_DEFAULT_PLACEHOLDER" | ||
METADATA_ENV_PREFIX = "DBT_ENV_CUSTOM_ENV_" | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import pytest | ||
import json | ||
import os | ||
|
||
from dbt.tests.util import run_dbt_and_capture | ||
|
||
|
||
def parse_json_logs(json_log_output): | ||
parsed_logs = [] | ||
for line in json_log_output.split("\n"): | ||
try: | ||
log = json.loads(line) | ||
except ValueError: | ||
continue | ||
|
||
parsed_logs.append(log) | ||
|
||
return parsed_logs | ||
|
||
|
||
class TestCustomVarInLogs: | ||
@pytest.fixture(scope="class", autouse=True) | ||
def setup(self): | ||
# on windows, python uppercases env var names because windows is case insensitive | ||
os.environ["DBT_ENV_CUSTOM_ENV_SOME_VAR"] = "value" | ||
yield | ||
del os.environ["DBT_ENV_CUSTOM_ENV_SOME_VAR"] | ||
|
||
def test_extra_filled(self, project): | ||
_, log_output = run_dbt_and_capture(['--log-format=json', 'deps'],) | ||
logs = parse_json_logs(log_output) | ||
for log in logs: | ||
assert log['info'].get('extra') == {"SOME_VAR": "value"} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This name might be natural in terms of some context I don't have yet, but I wonder if it can be improved. Maybe DBT_LOGGING_VAR_ or DBT_METADATA_VAR_?
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.
This is actually an existing name (docs). Metadata is in the manifest and these vars end up there currently. I centralized the constant into this (newish) file as part of my changes.
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.
It would be my preference to have a new key for this behavior, such as DBT_LOG_CUSTOM_ENV. Users already have existing DBT_ENV_CUSTOM_ENV environment variables, which they might actually be surprised to find in every log line. @jtcohen6 could you weigh in on this?
To me, this seems to be a different use case than we already have for DBT_ENV_CUSTOM_ENV.
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.
Conflating these two use cases also makes it impossible to put things in one place in the artifact output as opposed to in the artifact output plus every log line.
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'm not sure specifically how users are using
DBT_ENV_CUSTOM_ENV
currently.I do see what you're saying with users possibly being surprised these env vars show up in logs now. But is there a use case for these being the exact variables they want to show up? Definitely need @jtcohen6 to weigh in.
It's easy to separate out if that's the use case we want.
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.
Right now those DBT_ENV_CUSTOM_ENV variables show up in the artifacts that are written up in the end, and there was a recent change to put them in the context for access in Jinja.
I'm not familiar with how users might use those, but from some of our test cases, it does look like they were intended for putting things like a run_id or a job_id. So maybe it would be fine to use this prefix string.
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.
Thanks for tagging me in, and for the good conversation here!
Users set these env vars with the explicit intent of having them show up in metadata artifacts. It makes it easier to debug and quickly cross-reference. I see logs as a metadata artifact, in very much the same vein.
This was also our intent by crafting such an unwieldy prefix. It's hard to type
DBT_ENV_CUSTOM_ENV_
by accident. I'm definitely open to a better name (DBT_ENV_CUSTOM_METADATA_
?) that's equally clear about what it's doing. By convention, "functional" env vars should just be prefixed withDBT_
, including env vars setting global configs.I think it's an okay assumption to automatically coordinate between these. If later on, we hear a valid use case for having them show up in one, and not the other, I'm willing to admit that I was wrong! But I'd rather do this than require users to set two sets of identical env vars from the get-go.
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.
Thanks for the input @jtcohen6! I do think
DBT_ENV_CUSTOM_METADATA_
does a better job of saying what the var is for sure, so lean towards this new name.My understanding is that if we change the name we should add a deprecation warning for the old name and remove it in 2.0. My question is would
DBT_ENV_CUSTOM_ENV_
only show up in the artifacts (since that's maintaining current behavior) andDBT_ENV_CUSTOM_METADATA_
shows up in both logs and artifacts?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'd still vote in favor of coordinating the information so that it shows up in both places.
How would we feel about implementing this as
DBT_ENV_CUSTOM_ENV_
for now, and then opening a new ticket for the rename (+ deprecation warning)?