Log forwarding #153

cretz · 2023-11-08T23:01:34Z

What was changed

Added Temporalio.Runtime.LogForwardingConfig that accepts a ILogger to forward logs to
Added Temporalio.Runtime.LoggingOptions.Forwarding to accept the above
Added native layer to send logs to callback

Checklist

Closes [Feature Request] Add support for Log forwarding to lang-side #140

Sushisource

One main question.

Also I noticed we're not sending the span_contexts field from core (and I think I missed that in the Python PR). That's useful to send and have show up somehow as it provides a good bit of context (can kind of be considered part of the target)

Sushisource · 2023-11-09T00:00:51Z

src/Temporalio/Bridge/OptionsExtensions.cs

@@ -131,7 +131,8 @@ internal static class OptionsExtensions
            return new Interop.LoggingOptions()
            {
                filter = scope.ByteArray(options.Filter.FilterString),
-                forward = (byte)0,
+                // Forward callback is set in the Runtime constructor
+                // forward_to = ???


Leftover line?

Intentional, as a way to say "I know I am not setting this field here". Maybe ??? should be <set elsewhere>

EDIT: Updated

Sushisource · 2023-11-09T00:06:38Z

src/Temporalio/Bridge/src/runtime.rs

+    target: ByteArray,
+    message: ByteArray,
+    timestamp_millis: u64,
+    fields_json: ByteArray,


I'm confused why we're converting the fields to JSON here rather than passing something structured back into .NET? Can we not convert into some .NET object here and send that over in ForwardedLog rather than doing this serialization?

To pass something structured over the C FFI boundary means I build an entire object model in C to represent that. This usually means a recursive union with a new enum and figure out lifetime issues (vs copying). It's just more complicated, I don't already have serde_json::Value at my disposal in C like you have in Rust. It's a similar question to why I send proto bytes to/from core instead of creating structures to represent those.

But if it's worth it, I can do it. For now, I'm just converting to Dictionary<string, JsonElement> on the .NET side anyways. The other option I considered is to just pass a "fields_suffix" string for now that is built Rust side, but I figured I might as well provide the somewhat-structured field.

Yeah, I get that - I just thought it would probably be easy enough to make the right structure here since there's only a handful of types. I think it's probably worth it in the sense that it's not code we're likely to have any problem with, just a bit of extra boilerplate but it'll be quite a bit more efficient on a fairly hot path.

I don't think it's going to be that much more efficient since I am turning it back into a string anyways on the .NET side. So I guess I can just let the Rust side turn it into a string for me and then we'll have the best performance. The only reason to use structured data is if the user wants structured data. If we're concerned about performance, we should make the user decide to turn it to structured (which they can given a JSON string).

So now I've come full circle back to my current impl except not expose it as a Dictionary<string, JsonElement> but rather Dictionary<string, string> where values are JSON strings and if a user wants more structure, they can parse values.

Ok, I updated this to give keys and values where the values are raw JSON. This is the best performing I can do, and is better than converting each nested Rust object into a nested .NET object just to turn it to a string in the log message on the .NET side. Having Rust just serialize JSON is plenty fast for this use case (and not worth a new C map structure).

👍 makes sense to me. What I missed originally is that ILogger doesn't have any explicit structured k/v pairs in the interface anyway, so yeah there's no "place" to offer the structured objects.

there's no "place" to offer the structured objects.

Well, there is in that a logger gets an arbitrary "state" which is an enumerable of k/v pairs. So advanced users can use that to get the fields out (we do in our tests). But the default is to append the field values stringified anyways. So I'd prefer to make that the performant path and force users to deserialize JSON if they need more advanced structure if they happen to be doing something advanced with the values.

cretz · 2023-11-09T13:18:29Z

Also I noticed we're not sending the span_contexts field from core (and I think I missed that in the Python PR). That's useful to send and have show up somehow as it provides a good bit of context (can kind of be considered part of the target)

I left these off as I couldn't see a good way of them making it into log and I'm hesitant to bring in more data just for the log record if it's not on the default log message currently. But if they should be on the default log message it can make more sense, any suggestions of log message format welcome. Otherwise, I was just gonna wait until someone asked.

cretz · 2023-11-09T14:06:06Z

I am seeing some segfaults on Ubuntu ARM with the new logging test which is scary. I am moving this into draft mode while I do the push-debug-code-watch-CI dance to see if I can figure out what is happening.

cretz · 2023-11-13T22:37:57Z

Segfault lifetime issue fixed

Log forwarding support

cf45e50

Fixes temporalio#140

cretz requested a review from a team November 8, 2023 23:01

Sushisource reviewed Nov 9, 2023

View reviewed changes

Regen protos

7beb4f2

cretz marked this pull request as draft November 9, 2023 14:06

cretz force-pushed the log-forwarding branch 7 times, most recently from e842700 to c6477fd Compare November 13, 2023 20:25

cretz added 2 commits November 13, 2023 14:26

Fix logger lifetime issue

c6477fd

Changed JSON fields to be raw JSON

30aff36

cretz force-pushed the log-forwarding branch from aeb2253 to 30aff36 Compare November 13, 2023 22:22

cretz marked this pull request as ready for review November 13, 2023 22:36

Sushisource approved these changes Nov 13, 2023

View reviewed changes

Merge branch 'main' into log-forwarding

5e0856a

cretz merged commit 67a2572 into temporalio:main Nov 14, 2023

cretz deleted the log-forwarding branch November 14, 2023 14:44

cretz mentioned this pull request Dec 5, 2023

Sample request: Log forwarding temporalio/samples-dotnet#44

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log forwarding #153

Log forwarding #153

cretz commented Nov 8, 2023

Sushisource left a comment

Sushisource Nov 9, 2023

cretz Nov 9, 2023 •

edited

Loading

Sushisource Nov 9, 2023

cretz Nov 9, 2023 •

edited

Loading

Sushisource Nov 9, 2023

cretz Nov 9, 2023 •

edited

Loading

cretz Nov 13, 2023

Sushisource Nov 13, 2023

cretz Nov 14, 2023

cretz commented Nov 9, 2023 •

edited

Loading

cretz commented Nov 9, 2023

cretz commented Nov 13, 2023

Log forwarding #153

Log forwarding #153

Conversation

cretz commented Nov 8, 2023

What was changed

Checklist

Sushisource left a comment

Choose a reason for hiding this comment

Sushisource Nov 9, 2023

Choose a reason for hiding this comment

cretz Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Sushisource Nov 9, 2023

Choose a reason for hiding this comment

cretz Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Sushisource Nov 9, 2023

Choose a reason for hiding this comment

cretz Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

cretz Nov 13, 2023

Choose a reason for hiding this comment

Sushisource Nov 13, 2023

Choose a reason for hiding this comment

cretz Nov 14, 2023

Choose a reason for hiding this comment

cretz commented Nov 9, 2023 • edited Loading

cretz commented Nov 9, 2023

cretz commented Nov 13, 2023

cretz Nov 9, 2023 •

edited

Loading

cretz Nov 9, 2023 •

edited

Loading

cretz Nov 9, 2023 •

edited

Loading

cretz commented Nov 9, 2023 •

edited

Loading