-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
store backfill logs using instigation logger #22003
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @jamiedemaria and the rest of your teammates on |
8378bb2
to
f7d4185
Compare
if os.getenv("STORE_BACKFILL_LOGS", None): | ||
evaluation_time = pendulum.now("UTC") | ||
log_key = ["backfill", backfill_id, evaluation_time.strftime("%Y%m%d_%H%M%S")] | ||
with InstigationLogger( |
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.
the InstigationLogger
always uses the dagster
logger, whereas the backfill daemon uses the backfill-daemon-logger
super().__init__(name="dagster", level=coerce_valid_log_level(level)) |
likely need to make the name of the logger a parameter to instigation logger or something
@alangenfeld @prha marking this PR and some related ones as ready for review to get them into review queues. Similar deal as with the other prototype PRs, these aren't actually ready for merging, I just want to start getting feedback on this direction and figure out what problems exist that we need to address |
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 looks good to me
Same, I think this looks pretty clean. |
96a0354
to
25e5fec
Compare
25e5fec
to
b5a0491
Compare
Summary & Motivation
stores backfill logs using the InstigationLogger. gated on an instance method that we can override to control roll out.
related internal pr https://github.com/dagster-io/internal/pull/9879
How I Tested These Changes