Skip to content
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

Add logs to Dapr Workflows #645

Merged
merged 12 commits into from
Jan 10, 2024
Merged

Add logs to Dapr Workflows #645

merged 12 commits into from
Jan 10, 2024

Conversation

shivamkm07
Copy link
Contributor

Description

This PR does the following:

  1. Adds the support for adding logger in Dapr
  2. Adds logs in dapr-ext-workflow
  3. Passes the logger options to DTF-python to enable logging

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #626

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
@shivamkm07 shivamkm07 requested review from a team as code owners December 5, 2023 17:59
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (5c10b78) 86.43% compared to head (8eaba3c) 86.54%.

Files Patch % Lines
...orkflow/dapr/ext/workflow/dapr_workflow_context.py 66.66% 3 Missing ⚠️
...pr-ext-workflow/dapr/ext/workflow/logger/logger.py 88.46% 3 Missing ⚠️
...ext-workflow/dapr/ext/workflow/workflow_runtime.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
+ Coverage   86.43%   86.54%   +0.10%     
==========================================
  Files          75       78       +3     
  Lines        3893     3953      +60     
==========================================
+ Hits         3365     3421      +56     
- Misses        528      532       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

import logging


class LoggerOptions:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be the Logger class with init having the name parameter as required and rest as optional as they are now.
Then in one call logger = Logger(name) this is satisfied, is there a requirement that the logger must be initialized with the DaprWorkflowClient as seen in that file?

@berndverst thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mukundansundar Added logger class. Please review

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you are ready to refactor all other logging code for the rest of the Python SDKs please move the logger into a sub package of dapr-ext-workflow for now. (ext/dapr-ext-workflow sub folder)

@berndverst
Copy link
Member

My strong preference is for self contained PRs. So if you'd like to introduce a logger to be used for all Python SDKs, I would like to see that done in a single PR. That's why I am asking for the logger code to be moved to the workflow extension directory for now. As long as the code is self contained there I will approve :)

In the future we could still migrate that to be at the top level of the repo, but at that point I'd like the same PR to contain the switch to that logger throughout all SDKs.

Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
@shivamkm07
Copy link
Contributor Author

shivamkm07 commented Jan 2, 2024

My strong preference is for self contained PRs. So if you'd like to introduce a logger to be used for all Python SDKs, I would like to see that done in a single PR. That's why I am asking for the logger code to be moved to the workflow extension directory for now. As long as the code is self contained there I will approve :)

In the future we could still migrate that to be at the top level of the repo, but at that point I'd like the same PR to contain the switch to that logger throughout all SDKs.

Done. @berndverst Please review

Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall lgtm

Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
mukundansundar
mukundansundar previously approved these changes Jan 3, 2024
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
@berndverst berndverst merged commit 2c328d1 into dapr:main Jan 10, 2024
15 checks passed
litan1106 pushed a commit to litan1106/dapr-python-sdk that referenced this pull request Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

workflow Logs enhancements - Structured logging and right log levels
3 participants