-
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
docs: guide to set up custom logging in Dagster #24102
docs: guide to set up custom logging in Dagster #24102
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
994c82f
to
ddac10e
Compare
Graphite Automations"docs-beta - Assign Reviewers" took an action on this PR • (08/30/24)1 label was added and 2 reviewers were added to this PR based on Pedram Navid's automation. |
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 super handy, @mlarose.
My primary question is around if this also applies to custom log solutions like Datadog and Sentry, see comments. Thanks!
...cs_beta_snippets/docs_beta_snippets/guides/monitor-alert/custom-logging/asset-job-example.py
Outdated
Show resolved
Hide resolved
...ocs_beta_snippets/docs_beta_snippets/guides/monitor-alert/custom-logging/task-job-example.py
Outdated
Show resolved
Hide resolved
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.
Overall this looks good! I agree with @cmpadden 's sense that we should get rid of the "with_default_logger" thing, and just had some minor other comments
...es/docs_beta_snippets/docs_beta_snippets/guides/monitor-alert/custom-logging/customlogger.py
Outdated
Show resolved
Hide resolved
b3577aa
to
10e1104
Compare
I thought I had answered this but I don't see the comment, so risking sounding like a broken record, this only applies to how logs are formatted to either stdout or stderr. Log collectors that obtain logs from the stdout would see the change, but to my best knowledge, sentry is something that needs to be instrumented within the app and would require a different treatment. |
@cmpadden @OwenKephart - thanks for your comments. I believe I have addressed everything. |
10e1104
to
5eb5d49
Compare
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.
Great work. Thanks, @mlarose !
## Summary & Motivation Provide an updated guide in how to customize logging of jobs in Dagster. ## How I Tested These Changes - Manual testing in Dagster+ dogfood + dagster dev locally - BK ## Changelog [New | Bug | Docs] NOCHANGELOG
Summary & Motivation
Provide an updated guide in how to customize logging of jobs in Dagster.
How I Tested These Changes
Changelog [New | Bug | Docs]
NOCHANGELOG