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

Log forwarding #405

Closed
wants to merge 2 commits into from
Closed

Log forwarding #405

wants to merge 2 commits into from

Conversation

cretz
Copy link
Member

@cretz cretz commented Oct 24, 2023

What was changed

  • Added temporalio.runtime.LogBuffer which can be created for runtime and have retrieve_logs called on it
  • Added temporalio.runtime.LoggingConfig.buffer option to accept the above buffer
  • Added Rust code to performantly pass through core logs to the caller when they call retrieve_logs
  • Update pyo3 to 0.19 and added pythonize dependency to go from serde log fields to Python object
    • After some internal thought, the tiny size of that library coupled with the benefit of having additional fields was deemed worth it over converting to JSON on Rust side and from JSON on Python side. But can be convinced otherwise.

Checklist

  1. Closes [Feature Request] Log Forwarding Support for python-SDK #311

@cretz cretz requested a review from a team as a code owner October 24, 2023 15:17
@cretz cretz changed the title Python log forwarding Log forwarding Oct 24, 2023
@bergundy
Copy link
Member

Not sure how you're expecting users to use this. Are they supposed to fetch the buffered logs and feed those into their logger periodically?

@cretz
Copy link
Member Author

cretz commented Oct 24, 2023

Not sure how you're expecting users to use this. Are they supposed to fetch the buffered logs and feed those into their logger periodically?

Yes, same for buffered metrics (we even named everything the same). I could provide a default implementation for Python logging, but I hesitate to have a default frequency + asyncio-based sleep.

@bergundy
Copy link
Member

This isn't a good experience IMHO, we should be helping users collect these logs and make sense of the ordering between core and python logs.

@cretz
Copy link
Member Author

cretz commented Oct 24, 2023

This isn't a good experience IMHO, we should be helping users collect these logs and make sense of the ordering between core and python logs.

I disagree, I think we should give the user the ability to collect the logs and do what they want. This is the same with metrics buffering, it's just log buffering.

If we want to add layers on top to integrate with existing metrics systems or existing logging, I think that is a separate addition to this foundational piece. Not something that should block this piece

Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

I agree with Roey that this should be easier for users than having to do it themselves. I also think it's fine to not block the PR, but, releasing just this seems nowhere near as nice as releasing with the fully integrated version like TS has. It's not much extra to add either.

@@ -1,6 +1,7 @@
use pyo3::exceptions::{PyRuntimeError, PyValueError};
use pyo3::prelude::*;
use pyo3::AsPyPointer;
use pythonize::pythonize;
Copy link
Member

Choose a reason for hiding this comment

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

Nifty

@cretz
Copy link
Member Author

cretz commented Oct 24, 2023

The problem with moving logs from core to Python logging automatically is it requires polling, which is hard to guarantee in Python. Do I have a dedicated logging thread? Often not desirable. Or do I trust they will continually run the asyncio event loop given to me for the life of the logger? You might think that you're in a worker so it's ok, but lots of people are starting these asyncio loops just for one client call and terminating them.

Also there's of course the challenges with log ordering. I will have to research what TS does there and what kind of latency/backlog we'd tolerate to do stream sorting.

I have opened #407 to track it, but for some use cases from users, this PR unblocks them.

@cretz
Copy link
Member Author

cretz commented Oct 25, 2023

I may hold off on merging this. We may want a model where Rust pushes a notification once it has buffered logs to be drained based on temporalio/sdk-core#618.

@@ -220,6 +225,93 @@ def retrieve_updates(self) -> Sequence[BufferedMetricUpdate]:
return self._runtime._core_runtime.retrieve_buffered_metrics()


class LogBuffer:
"""A buffer that can be set on :py:class:`LoggnigConfig` to record logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""A buffer that can be set on :py:class:`LoggnigConfig` to record logs
"""A buffer that can be set on :py:class:`LoggingConfig` to record logs

@cretz
Copy link
Member Author

cretz commented Oct 31, 2023

Ok, this is not going to work. The team wants (buffered) logs pushed to Python logging without this flexibility. Closing.

@cretz cretz closed this Oct 31, 2023
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.

[Feature Request] Log Forwarding Support for python-SDK
4 participants