CP-44752: propagate System.Diagnostics tracing information using W3C traceparent header #5929

edwintorok · 2024-08-12T14:30:57Z

The newer System.Net.Http would propagate this automatically, but the older WebRequest doesn't.

Retrieve the span identifier and add the header ourselves. The WebRequest object is fresh for each request, so setting the headers shouldn't cause race conditions.

This adds a dependency on a library that is part of .Net source repository, but distributed outside of it on NuGet: System.Diagnostics. Lock the dependency to a given hash to avoid accidental/malicious changes.
This dependency is not available on .Net45, so all the code and build dependencies are wrapped with appropriate conditional compilation directives (which may look kind of ugly, they can be dropped once XenServer.dll drops support for .Net45 and switches to .Net462 minimum).

No new instrumentation is created, so if the application using XenServer.dll is not configured/set up to use OpenTelemetry or System.Diagnostics.DiagnosticSource, then this is a no-op.

I've tested this using the Powershell 7 module.

Draft PR because:

I haven't tested the version with the locked dependencies yet
There is one TODO in the code about obtaining a version number

…ency This only exists on .Net 4.6+, so add it conditionally only when target >Netstandard 2.0 (which includes .Net 4.6). This allows creating activity spans, independently of the tracing framework used. Signed-off-by: Edwin Török <[email protected]>

edwintorok · 2024-08-12T14:57:20Z

Internal build succeeded with locked package dependencies too.

edwintorok · 2024-08-12T16:15:11Z

I think I'll also need NET462_OR_GREATER otherwise XC won't pick this up.

Doesn't exist on .Net45. Signed-off-by: Edwin Török <[email protected]>

This is not available on .Net 4.5 Signed-off-by: Edwin Török <[email protected]>

To avoid someone injecting a malicious binary with the same version in the future, lock all dependencies. Signed-off-by: Edwin Török <[email protected]>

danilo-delbusso

haven't tested it yet but I spotted a couple of things on the first run

for SDK PRs also please request reviews manually, because we don't monitor the repo as often as toolstack does, so it helps to see PR reviews under https://github.com/pulls

danilo-delbusso · 2024-08-21T09:56:29Z

ocaml/sdk-gen/csharp/autogen/src/JsonRpc.cs

@@ -155,6 +158,11 @@ public partial class JsonRpcClient
    {
        private int _globalId;

+#if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER)
+        // TODO: get proper version string from build


We never merge source with TODOs on master.

Use the name of the package XenServer.NET and its version. I think we can get it from https://learn.microsoft.com/en-us/dotnet/api/system.reflection.assemblyname.version?view=netstandard-2.0, but needs testing

If not, this needs a placeholder, and changes to pipelines. We can advise on that if needed

If the client code uses the C# SDK as a project in their code, typeof(JsonRpcClient).Assembly.GetName().Version should return the SDK version, but in the corner case of someone dropping the sources in a client project (like XenCenter does), it should return the client project version (in XC it would return XenModel's version, which is XC's version).

If we can't have the ocaml build use a placeholder and replace it with the tag (we ran into some trouble with @psafont when we tried it for the API reference), I suggest using new ActivitySource("XenAPI.JsonRpcClient", "@SDK_VERSION@") (the truth is I dislike placeholders like this, but we have one more insance of it in the PS code).

By the way, is it indeed the SDK version we need here? (As opposed to a string that will be representing the implementing client, somehting like the UserAgent, for example).

but in the corner case of someone dropping the sources in a client project (like XenCenter does)

we could do without the placeholder and just add a line in the README 🤷

By the way, is it indeed the SDK version we need here? (As opposed to a string that will be representing the implementing client, somehting like the UserAgent, for example).

for this, I'd say so

if a client wants to use open telemetry, it should add it itself and then use its own user agent

I may be wrong - I'm not that familiar with what the expected behaviour is

it should add it itself and then use its own user agent

Yes, the thing is we offer the option to use own UserAgent (albeit it sub-optimally designed in my opinion), but we don't offer this option for the activity source with this design.

danilo-delbusso · 2024-08-21T09:57:37Z

ocaml/sdk-gen/csharp/autogen/src/JsonRpc.cs

@@ -225,6 +241,10 @@ protected virtual T Rpc<T>(string callName, JToken parameters, JsonSerializer se

                    using (var responseStream = new MemoryStream())
                    {
+#if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER)
+                        // https://opentelemetry.io/docs/specs/semconv/rpc/json-rpc/


please add a comment explaining the tag within the source. links can change and not work anymore. If you still think it'd be useful to have a link, I'd use https://archive.org/

danilo-delbusso · 2024-08-21T09:57:51Z

ocaml/sdk-gen/csharp/autogen/src/JsonRpc.cs

+                                        if (errorArray != null) {
+#if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER)
+                                            activity?.SetTag("otel.status_code", "ERROR");
+                                            //activity?.SetTag("rpc.jsonrpc.error_message", errorArray);


Suggested change

//activity?.SetTag("rpc.jsonrpc.error_message", errorArray);

Should we be imlementing this properly rather than commenting it out - currently there is disparity between v1 and v2 implementation.

danilo-delbusso · 2024-08-21T09:58:52Z

ocaml/sdk-gen/csharp/autogen/src/JsonRpc.cs

-                                        if (errorArray != null)
+                                        if (errorArray != null) {
+#if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER)
+                                            activity?.SetTag("otel.status_code", "ERROR");


I'd use const for all these strings here (such as otel.status_code, ERROR, etc.)

kc284 · 2024-08-22T08:01:04Z

ocaml/sdk-gen/csharp/autogen/src/JsonRpc.cs

@@ -239,12 +259,25 @@ protected virtual T Rpc<T>(string callName, JToken parameters, JsonSerializer se
 #else
                                    var res2 = (JsonResponseV2<T>)serializer.Deserialize(responseReader, typeof(JsonResponseV2<T>));
 #endif
+
+#if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER)
+                                    activity?.SetTag("rpc.jsonrpc.version", "2.0");


Shouldn't we be setting this tag further down for v1 too?

kc284 · 2024-08-22T08:02:07Z

ocaml/sdk-gen/csharp/autogen/src/JsonRpc.cs

+            // propagate W3C traceparent and tracestate
+            // HttpClient would do this automatically on .NET 5,
+            // and .NET 6 would provide even more control over this: https://blog.ladeak.net/posts/opentelemetry-net6-httpclient
+            // the caller must ensure that the activity is in W3C format (by inheritance or direc setting)


typo: direc->direct

kc284

As above.

kc284 added the sdk-members-only label Aug 12, 2024

edwintorok marked this pull request as ready for review August 12, 2024 14:57

edwintorok added 3 commits August 12, 2024 17:17

SDK(C#): Conditional activity source for JsonRPC

8ef759b

Doesn't exist on .Net45. Signed-off-by: Edwin Török <[email protected]>

SDK(C#): Conditionally propagate W3C traceparent

3f7d8e6

This is not available on .Net 4.5 Signed-off-by: Edwin Török <[email protected]>

SDK(C#): lock package dependencies to a specific hash

3037f81

To avoid someone injecting a malicious binary with the same version in the future, lock all dependencies. Signed-off-by: Edwin Török <[email protected]>

edwintorok force-pushed the private/edvint/otelsdk branch from c2723ca to 3037f81 Compare August 12, 2024 16:17

danilo-delbusso requested review from kc284 and danilo-delbusso and removed request for kc284 August 21, 2024 09:51

danilo-delbusso requested changes Aug 21, 2024

View reviewed changes

danilo-delbusso assigned danilo-delbusso and edwintorok and unassigned danilo-delbusso Aug 21, 2024

danilo-delbusso requested a review from kc284 August 21, 2024 09:59

kc284 reviewed Aug 22, 2024

View reviewed changes

kc284 requested changes Aug 22, 2024

View reviewed changes

kc284 added the needs updating label Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-44752: propagate System.Diagnostics tracing information using W3C traceparent header #5929

CP-44752: propagate System.Diagnostics tracing information using W3C traceparent header #5929

edwintorok commented Aug 12, 2024

edwintorok commented Aug 12, 2024

edwintorok commented Aug 12, 2024

danilo-delbusso left a comment

danilo-delbusso Aug 21, 2024

kc284 Aug 22, 2024

danilo-delbusso Aug 23, 2024

danilo-delbusso Aug 23, 2024

kc284 Aug 27, 2024

danilo-delbusso Aug 21, 2024

danilo-delbusso Aug 21, 2024

kc284 Aug 22, 2024

danilo-delbusso Aug 21, 2024 •

edited

Loading

kc284 Aug 22, 2024

kc284 Aug 22, 2024

kc284 left a comment

CP-44752: propagate System.Diagnostics tracing information using W3C traceparent header #5929

Are you sure you want to change the base?

CP-44752: propagate System.Diagnostics tracing information using W3C traceparent header #5929

Conversation

edwintorok commented Aug 12, 2024

edwintorok commented Aug 12, 2024

edwintorok commented Aug 12, 2024

danilo-delbusso 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

danilo-delbusso Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kc284 left a comment

Choose a reason for hiding this comment

danilo-delbusso Aug 21, 2024 •

edited

Loading