-
Notifications
You must be signed in to change notification settings - Fork 24
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
Racheld/structured logging discovery #131
Conversation
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. |
|
||
from dbt.events.functions import STDOUT_LOG, FILE_LOG | ||
import dbt.logger as dbt_logger | ||
from dbt.events.functions import EVENT_MANAGER |
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.
@peterallenwebb we probably want to move EVENT_MANAGER into a contextvar for this kind of usecase? If we plan to have concurrent request being handled by the same dbt-server container
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.
@ChenyuLInx Thanks for opening dbt-labs/dbt-core#6399! Love to see it
line_format=LineFormat.DebugText, | ||
level=EventLevel.INFO, | ||
use_colors=True, | ||
output_file_name=log_path, |
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.
For my understanding, We are writing the logs to a local file then stream-log
endpoint gonna read that file and send SSE to WS controller?
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.
Yes, that is currently how it's working! This is just bringing us to parity with how the server logging worked before, we aren't set on this implementation.
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.
My goal here would be: finding a straightforward way to get this working for now, while preserving future options where we push events elsewhere / elsehow. Sounds like this approach fits the bill
What is this PR?
This is a:
All pull requests from community contributors should target the
main
branch (default).Description & motivation
Checklist