Conversation
GSmithApps
force-pushed
the
changle_log_level
branch
from
5cfa4c5 to
598c1af
Compare
hello/hello_change_log_level.py
Outdated
| return True | ||
|
|
||
|
|
||
| logging.getLogger("temporalio.worker._workflow_instance").addFilter(CustomLogFilter()) |
There was a problem hiding this comment.
| logging.getLogger("temporalio.worker._workflow_instance").addFilter(CustomLogFilter()) | |
| logging.getLogger().addFilter(CustomLogFilter()) |
Can we add this to the root logger instead (untested)? There are no guarantees about how we may refactor internals.
There was a problem hiding this comment.
Thank you very much for catching this! I believe it is solved 👍
There was a problem hiding this comment.
May be able to add a test for this to confirm continued expected behavior (though I know we haven't always added tests for all samples)
There was a problem hiding this comment.
Thanks for keeping me honest!
Speaking of that, I vibe coded a test that that checks what we want: it passes with the code as-is, but it fails if I remove the sample's configuration to elevate the logging.
hello/hello_change_log_level.py
Outdated
| from temporalio.worker import Worker | ||
|
|
||
| # --- Begin logging set‑up ---------------------------------------------------------- | ||
| _WORKFLOW_TASK_FAILURE_LOG_PREFIX = "Failed activation on workflow" |
There was a problem hiding this comment.
@cretz I'm a little worried about this hardcoding cuz I'm not sure we guarantee this log message won't be modified.
- If it won't be modified, I think we don’t have anything to worry about
- if it could be modified, could we do something like expose a constant from the SDK? I recognize it might be a little goofy, but it might be more robust
There was a problem hiding this comment.
Yes, this is exactly why we just solved temporalio/sdk-python#864 last week. I'd wait until that is released (soon).
There was a problem hiding this comment.
Thanks @cretz! I see the new SDK was released and includes the fix (888). Judging by the files changed in the PR, this string didn’t change, correct? So the benefit of the PR is adding the test to reinforce the fact that this string is public API and won’t change? Meaning, the sample is still good? I know you said nothing blocking
There was a problem hiding this comment.
The message string didn't change, but the purpose of the issue is to add another, more stable string for this use case. So we should encourage in this sample to check __temporal_error_identifier attribute instead.
There was a problem hiding this comment.
Thanks — that looked private to me, but upon further inspection, it’s okay to use. Done ✅
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM, nothing really blocking, but let's wait for the impending Python SDK release so you can use a more deliberate differentiator on the log record.
hello/hello_change_log_level.py
Outdated
| from temporalio.worker import Worker | ||
|
|
||
| # --- Begin logging set‑up ---------------------------------------------------------- | ||
| _WORKFLOW_TASK_FAILURE_LOG_PREFIX = "Failed activation on workflow" |
There was a problem hiding this comment.
Yes, this is exactly why we just solved temporalio/sdk-python#864 last week. I'd wait until that is released (soon).
hello/hello_change_log_level.py
Outdated
| id="hello-workflow-id", | ||
| task_queue="hello-task-queue", |
There was a problem hiding this comment.
Would recommend an ID and task queue named specifically for this sample
There was a problem hiding this comment.
Love it - fixed 👍
|
I believe all comments are resolved and this is ready. |
|
@cretz ready if you are 👍 |
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
I still think this is a bit too specific of a need to go with all the other hello things, but meh
2 participants
What was changed
Added a hello sample showing how to change the log level for workflow task failures
Why?
Some users have requested a sample on this.
Checklist
Any docs updates needed?
Updated the top-level readme and the hello readme
How was this tested:
Ran and confirmed that the log level was changed to error