-
Notifications
You must be signed in to change notification settings - Fork 162
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
move job link logging just after the job submit #697
Conversation
and query_job.job_id is not None | ||
and query_job.project is not None | ||
): | ||
logger.debug( |
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.
hey @Kayrnt! what do you think about making this an info
level log?
this job URL is super useful even beyond debugging code, since it's the ground truth of what logic and process is actually executing the model. I think it should be more readily available, understanding that most apps out there probably wouldn't enable debug
logs in production
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.
Hello @robsicurelli, I agree with you that it's making the logs unnecessarily verbose if you'd like to keep BQ links in your scheduler logs all the time.
I'm fine moving it to info
but I'll let @Fleid call the shot on that one 👍
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 against it, but that should be made into its own issue/PR rather than addressed here.
We need a visible comment section for people to complain, and an easy way to revert if there's unintended consequences ;)
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.
@Fleid that sounds good to me. I'm happy to stamp this PR as-is, and open another issue for changing the log level
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.
LGTM
resolves #696
Description
So far the job link is logged once the fails or if the debug mode is enabled and the job is done.
However in a lot of cases, if you want to follow a job execution for a long running one (ie to look at execution plan or access the UI to cancel it manually), you have to find it again by using the job search on the UI/CLI which is inconvenient.
There is some small side effects because of that change:
I think it's totally acceptable but feel free to challenge the design.
Checklist
changie new
to create a changelog entry