Log forwarding support #413
Conversation
| // Start log forwarding if needed | ||
| let log_forwarder_handle = log_forwarding.map(|(stream, callback)| { | ||
| Arc::new(core.tokio_handle().spawn(async move { | ||
| let mut stream = std::pin::pin!(stream.chunks_timeout( |
There was a problem hiding this comment.
Hmm... only issue I see with this approach is that there's no throttling, just batching.
We should protect lang and ensure that we drop messages if they're produced too fast.
I don't think chunks_timeout is what we want.
You can merge an IntervalStream with the log stream and have an accumulator that flushes when it gets a interval input and drops messages after the buffer fills up.
There was a problem hiding this comment.
We should protect lang and ensure that we drop messages if they're produced too fast.
I documented this behavior and have it this way by intention. If we must protect lang with too-frequent invocations, I think we should do it by exposing the buffer size and interval. I could just as easily say "we should protect lang by never dropping logs" which I think is a valid stance and the stance that this PR takes. I know TypeScript chose to silently drop logs but I don't think we need to accept that as the right behavior.
I'm actually a little worried I can't configure TS to not drop logs if I am a user (and willing to accept whatever penalty to not be lossy). We batch as a courtesy, I was hoping I wasn't going to have to benchmark to check whether Python can handle getting constant streams of 2048 logs at a time, but it might be able to with backpressure into Rust a bit.
There was a problem hiding this comment.
I think in practice we probably don't lose any logs in TS unless you're running with TRACE levels for all of the Rust logs so it's probably acceptable not to drop logs the way that you're set things up here.
We can start with this and add throttling if log processing load becomes an issue.
There was a problem hiding this comment.
👍 Will merge, but definitely agree we can increase flexibility in either way later.
| logger: logging.Logger | ||
| """Core logger messages will be sent to this logger.""" |
There was a problem hiding this comment.
One thing here, you might want to consider emitting to a logger with the module name. That's fairly standard Python practice.
There was a problem hiding this comment.
We do that for how we use Python logging, but here users choose the logger to provide, so they can provide a logger with whatever name they want and whatever configuration they want. I also append the core log target to give logger name by default, but I don't want to be in charge of creating and managing loggers (or assuming I know how they want to configure their logger or name it or anything).
There was a problem hiding this comment.
This is very non Pythonic practice based on my experience.
I'd at minimum mark this API experimental.
There was a problem hiding this comment.
I don't think it's non-Pythonic to accept a user-configured logger from a user configured for pushed external logging like this. It's bad to assume that this special case of using an logger may not want to be configured in a custom way from other logging. It's not like we do this for our normal logs, this is a very special case and they may want to customize the logger for this case.
Can you clarify exactly what you think should change? Should we use the global default logger factory? Should we continue appending the core target to the name of the log records? You think users should be able to explicitly set handlers for core logging?
I think you need better justifications and clarifications here. You can't assume your experience in normal logging situations applies to pushed logs in this special circumstance.
I'd at minimum mark this API experimental.
EDIT: done in #418. But we really need something more concrete here or it will just remain this way.
There was a problem hiding this comment.
My suggestion is that you emit to a logger that you create (e.g. logging.getLogger(__name__)) and let users handle logs as they normally would with a global logging configuration.
That way, all they need to do is opt-in to forwarding, and the rest "just works".
There was a problem hiding this comment.
The difference between LogForwardingConfig(forwarding = True) and LogForwardingConfig(logger = logging.getLogger("my-name")) is negligible for "just works".
Now, where is this logger stored? How do I add a handler to that logger? How do I have different runtimes have different loggers? Should I append the Core target to that name on each log record? Is the only reason you're wanting to do this is because True appears easier than logging.getLogger("my-name") from a user POV regardless of all of the known negatives?
|
Thanks @cretz! Which release will include these changes? |
|
The next one, 1.4.0, hopefully coming soon. |
What was changed
temporalio.runtime.LogForwardingConfigwhich can be set ontemporalio.runtime.LoggingConfig.forwardingto enable thislogging.Loggerand sends callback to core for accepting core logstemporal_logfield on the log record with the core log representationChecklist