Open Telemetry prototype #769

jskeet · 2024-03-13T11:26:01Z

The intention of this PR is that we merge some parts of it, but not all of it - ideally whole commits should be in or out.

amanda-tarafa

I've left a few comments, but of the things we knew we had to chat about anyway.

amanda-tarafa · 2024-03-14T05:36:10Z

Google.Api.Gax/VersionHeaderBuilder.cs

+        /// <summary>
+        /// Formats the version of the assembly containing the specified type, in a "generally informative" way.
+        /// </summary>
+        internal static string FormatAssemblyVersion(System.Type type)


This is a nit, but is there somewhere else we can move this method to? A general purpose utils class or similar? It's just weird to call VersionHeaderBuilder.FormatAssemblyVersion from the CreateActivitySource method.

I agree, but I don't have a better place for now. Will add a TODO for now. One thing that's worth noting is that the "is it a valid header value" may not be relevant in a more general case (and may not be reasonable for OTel, even).

amanda-tarafa · 2024-03-14T05:37:28Z

Google.Api.Gax/ActivitySources.cs

+    /// Returns an <see cref="ActivitySource"/> named after the given client type.
+    /// </summary>
+    /// <remarks>
+    /// Multiple calls to this method (or <see cref="FromClientType{T}()"/>) for the same


I can't leave a suggestion but on this one:

FromClientType{T} => FromClientType(System.Type)

amanda-tarafa · 2024-03-14T05:38:19Z

Google.Api.Gax/ActivitySources.cs

+    /// </remarks>
+    /// <param name="type">The client type to obtain an activity source for. Must not be null.</param>
+    /// <returns>An <see cref="ActivitySource"/> named after the given client type.</returns>
+    public static ActivitySource FromClientType(System.Type type)


I think I'd drop the Client from the method name. We are not enforcing that, so these methods could be used to get activity sources for any type.

I was wondering about that. They could be used for any type, but we don't want to give the impression that they would be used by any other system by default. Let's talk about it more - I know you're not 100% convinced by having this at all.

amanda-tarafa · 2024-03-14T05:41:32Z

Google.Api.Gax.Grpc/ApiCallTracingExtensions.cs

+internal static class ApiCallTracingExtensions
+{
+    // Unary async
+    internal static Func<TRequest, CallSettings, Task<TResponse>> WithRetry<TRequest, TResponse>(


This should be named WithRetry => WithTracing

Doh, thanks. Done.

amanda-tarafa · 2024-03-14T05:50:14Z

Google.Api.Gax.Grpc/ClientHelper.cs

@@ -104,6 +108,7 @@ public ClientHelper(Options options)
            // These operations are applied in reverse order.
            // I.e. Version header is added first, then retry is performed.
            return ApiCall.Create(methodName, asyncGrpcCall, syncGrpcCall, baseCallSettings, Clock)
+                .WithTracing(_activitySource)


I think we should call WithTracing after calling WithLogging, so that logs are withtin the activity.

Thereby hangs a tail... currently we're using the method name of the delegate in WithTracing to get the activity name, which would be messed up by WithLogging. We should almost certainly stop doing that, basically - it makes all of this code really brittle. But we can decide all of that when we revisit the activity (and activity source) name.

Ah yes! Definetely a strong reason to stop using the name of the delegate in my view. As in I think it's more important (an expected) that the logs are contained in the trace. The methods themselves can access the current activity and set their own name as a tag, so we can still make that information available easily.

amanda-tarafa · 2024-03-14T06:07:45Z

Google.Api.Gax.Grpc/ClientHelper.cs

@@ -104,6 +108,7 @@ public ClientHelper(Options options)
            // These operations are applied in reverse order.
            // I.e. Version header is added first, then retry is performed.
            return ApiCall.Create(methodName, asyncGrpcCall, syncGrpcCall, baseCallSettings, Clock)
+                .WithTracing(_activitySource)
                .WithLogging(Logger)
                .WithRetry(Clock, Scheduler, Logger)


And I think we should maybe change WithRetry so that it also receives the activity source, and creates an activity per attempt.
Alternatively, we call WithTracing after WithRetry so that the whole thing, including retries are within a single activity, and the logs from retrying are withing our activity.
Alternatively, we have a single activity that contains the retries, and each attempt is an event?
I think I'd prefer the first one, but any of these seem fine, as long as attempts are explicitly included in the trace.

Google.Api.Gax.Grpc/ApiCallTracingExtensions.cs

amanda-tarafa · 2024-03-14T06:10:08Z

Google.Api.Gax.Grpc/ApiCallTracingExtensions.cs

+        return async (request, callSettings) =>
+        {
+            using var activity = activitySource.StartActivity(activityName, ActivityKind.Client);
+            activity?.SetTag(GrpcCallTypeTag, UnaryCallType);


I think we should have a tag with the name of the client, in case users pass in their own activity source, we may want to still be able to identify this activity as produced by us.

Maybe. I've added a TODO so that we remember to look at it - basically I think we'll need a whole design doc just about what names/tags we want to expose.

Yes, agreed we need to desing this elsewhere: naming, tags, and events and spans, and what corresponds to what, for instance, is a retry attempt represented in it's own span or is it an event? And what name it has, etc.

amanda-tarafa · 2024-03-14T06:12:39Z

Google.Api.Gax.Grpc/ApiCallTracingExtensions.cs

+    // (e.g. the full RPC name, the client name etc).
+    private static string FormatActivityName(Delegate fn, string methodName) => $"{fn.Method.Name}/{methodName}";
+
+    // TODO: See if there's a standard way of doing this. It seems odd to have to do it ourselves.


I was thinking the same.
This actually looks more like a logging concern than a tracing one? But yes, let's see what others do or if there are best practices etc.

I think setting the status to be an error is probably important for tracing though.

Yep, the status is relevant, but the exception message and the stack trace seem odd in tracing, at least to me.

amanda-tarafa · 2024-03-14T06:16:54Z

Google.Api.Gax.Grpc/ApiCallTracingExtensions.cs

+            }
+            catch (Exception ex) when (SetActivityException(activity, ex))
+            {
+                // We'll never actually get here, because SetActivityException always returns false.


I think it'll be way clearer to have the SetActivityException method be void, and just call it here, and then thrwo. And that's not just because that's actually what happens, but also because the bool SetActivityException combination makes me thingkthat this method will return true if it set the info, and false otherwise, when what happens is, it always sets the info but always returns false.

I was worried about throw losing context in some async situations.

We could use ExceptionDispatchInfo.Capture(e.InnerExceptions.FirstOrDefault() ?? e).Throw(); instead, perhaps.
(I'm aware that using an exception filter here is a bit odd.)

Will add a comment so we don't forget to come back to this.

…pi.Gax

This wires everything up, but doesn't actually start any activities. The implementation will all be in ApiCallTracingExtensions, in another commit.

While we *could* remove ActivitySource from ClientHelper.Options, that would require the settings to be cloned if the user hasn't specified an activity source. That could potentially be done in ClientBuilderBase, but keeping the default separate from the user-specified activity source feels simpler.

There's just a single test so far - more required. We should discuss this commit in detail before going further. Streaming calls are trickier, because we don't want to stop the activity before the call has actually completed. (We may not even know that in some cases.)

jskeet · 2024-03-14T08:17:12Z

New commits pushed - I hope they don't mess up the context of the comments too much.

amanda-tarafa

This looks good, I think we mostly agree on everything including in what needs further discussion. And thanks for the TODOs!

amanda-tarafa · 2024-03-18T05:27:39Z

Google.Api.Gax.Grpc/ClientHelper.cs

@@ -104,6 +108,7 @@ public ClientHelper(Options options)
            // These operations are applied in reverse order.
            // I.e. Version header is added first, then retry is performed.
            return ApiCall.Create(methodName, asyncGrpcCall, syncGrpcCall, baseCallSettings, Clock)
+                .WithTracing(_activitySource)


Ah yes! Definetely a strong reason to stop using the name of the delegate in my view. As in I think it's more important (an expected) that the logs are contained in the trace. The methods themselves can access the current activity and set their own name as a tag, so we can still make that information available easily.

amanda-tarafa · 2024-03-18T05:30:31Z

Google.Api.Gax.Grpc/ClientHelper.cs

@@ -214,7 +214,8 @@ public sealed class Options
            public string ApiVersion { get; set; }

            /// <summary>
-            /// The activity source to use for tracing, if any. This may be null.
+            /// The activity source to use for tracing, if any. This may be null. This is ignored
+            /// if <see cref="Settings"/> specifies an activity source.


Ah yes, this always trips me, even when I'm writing docs myself.

I think as is is fine then.

amanda-tarafa · 2024-03-18T05:31:28Z

Google.Api.Gax.Grpc/ApiCallTracingExtensions.cs

+        return async (request, callSettings) =>
+        {
+            using var activity = activitySource.StartActivity(activityName, ActivityKind.Client);
+            activity?.SetTag(GrpcCallTypeTag, UnaryCallType);


Yes, agreed we need to desing this elsewhere: naming, tags, and events and spans, and what corresponds to what, for instance, is a retry attempt represented in it's own span or is it an event? And what name it has, etc.

amanda-tarafa · 2024-03-18T05:35:38Z

Google.Api.Gax.Grpc/ApiCallTracingExtensions.cs

+    // (e.g. the full RPC name, the client name etc).
+    private static string FormatActivityName(Delegate fn, string methodName) => $"{fn.Method.Name}/{methodName}";
+
+    // TODO: See if there's a standard way of doing this. It seems odd to have to do it ourselves.


Yep, the status is relevant, but the exception message and the stack trace seem odd in tracing, at least to me.

jskeet · 2024-03-18T06:46:10Z

Great, thanks. We can talk in a VC about how we want to make progress. (We could potentially make everything internal and start merging the bits we're happier with, for example.)

jskeet assigned amanda-tarafa Mar 13, 2024

jskeet requested a review from a team as a code owner March 13, 2024 11:26

amanda-tarafa reviewed Mar 14, 2024

View reviewed changes

jskeet force-pushed the open-telemetry branch from c8cb76d to a82c469 Compare March 14, 2024 08:16

jskeet added 5 commits March 14, 2024 08:16

chore: Add System.Diagnostics.DiagnosticSource dependency to Google.A…

b091a48

…pi.Gax

feat: Add helper methods to obtain ActivitySources

8c7bc61

feat: Support ActivitySource in ClientHelper

cf86322

This wires everything up, but doesn't actually start any activities. The implementation will all be in ApiCallTracingExtensions, in another commit.

jskeet force-pushed the open-telemetry branch from a82c469 to 57c7919 Compare March 14, 2024 08:17

amanda-tarafa reviewed Mar 18, 2024

View reviewed changes

googleapis deleted a comment May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Telemetry prototype #769

Open Telemetry prototype #769

jskeet commented Mar 13, 2024

amanda-tarafa left a comment

amanda-tarafa Mar 14, 2024

jskeet Mar 14, 2024

amanda-tarafa Mar 14, 2024

jskeet Mar 14, 2024

amanda-tarafa Mar 14, 2024

jskeet Mar 14, 2024

amanda-tarafa Mar 14, 2024

jskeet Mar 14, 2024

amanda-tarafa Mar 14, 2024 •

edited

Loading

jskeet Mar 14, 2024

amanda-tarafa Mar 18, 2024

amanda-tarafa Mar 14, 2024 •

edited

Loading

amanda-tarafa Mar 14, 2024

jskeet Mar 14, 2024

amanda-tarafa Mar 18, 2024

amanda-tarafa Mar 14, 2024

jskeet Mar 14, 2024

amanda-tarafa Mar 18, 2024

amanda-tarafa Mar 14, 2024

jskeet Mar 14, 2024

jskeet commented Mar 14, 2024

amanda-tarafa left a comment

amanda-tarafa Mar 18, 2024

amanda-tarafa Mar 18, 2024

amanda-tarafa Mar 18, 2024

amanda-tarafa Mar 18, 2024

jskeet commented Mar 18, 2024

Open Telemetry prototype #769

Are you sure you want to change the base?

Open Telemetry prototype #769

Conversation

jskeet commented Mar 13, 2024

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Mar 14, 2024

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Mar 18, 2024

amanda-tarafa Mar 14, 2024 •

edited

Loading

amanda-tarafa Mar 14, 2024 •

edited

Loading