CP-44752: propagate System.Diagnostics tracing information using W3C traceparent header #5929
+150
−1
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -29,6 +29,9 @@ | |||
|
|
||||
| using System; | ||||
| using System.Collections.Generic; | ||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| using System.Diagnostics; | ||||
| #endif | ||||
| using System.IO; | ||||
| using System.Net; | ||||
| using System.Net.Security; | ||||
|
|
@@ -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 | ||||
| private static ActivitySource source = new ActivitySource("XenAPI.JsonRpcClient", "1.0"); | ||||
| #endif | ||||
|
|
||||
| public JsonRpcClient(string baseUrl) | ||||
| { | ||||
| Url = baseUrl; | ||||
|
|
@@ -207,6 +215,14 @@ protected virtual T Rpc<T>(string callName, JToken parameters, JsonSerializer se | |||
| // therefore the latter will be done only in DEBUG mode | ||||
| using (var postStream = new MemoryStream()) | ||||
| { | ||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| using (Activity activity = source.CreateActivity("XenAPI" + callName, ActivityKind.Client)) | ||||
| { | ||||
| // .NET 5 would use W3C format for the header by default but we build for .Net 4.x still | ||||
| activity?.SetIdFormat(ActivityIdFormat.W3C); | ||||
| activity?.Start(); | ||||
| activity?.SetTag("rpc.jsonrpc.request_id", id); | ||||
| #endif | ||||
| using (var sw = new StreamWriter(postStream)) | ||||
| { | ||||
| #if DEBUG | ||||
|
|
@@ -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/ | ||||
|
There was a problem hiding this comment. 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/ |
||||
| activity?.SetTag("rpc.method", callName); | ||||
| #endif | ||||
| PerformPostRequest(postStream, responseStream); | ||||
| responseStream.Position = 0; | ||||
|
|
||||
|
|
@@ -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"); | ||||
|
There was a problem hiding this comment. Shouldn't we be setting this tag further down for v1 too? |
||||
| #endif | ||||
| if (res2.Error != null) | ||||
| { | ||||
| var descr = new List<string> { res2.Error.Message }; | ||||
| descr.AddRange(res2.Error.Data.ToObject<string[]>()); | ||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| activity?.SetTag("otel.status_code", "ERROR"); | ||||
| activity?.SetTag("rpc.jsonrpc.error_code", res2.Error.Code); | ||||
| activity?.SetTag("rpc.jsonrpc.error_message", descr); | ||||
| #endif | ||||
| throw new Failure(descr); | ||||
| } | ||||
|
|
||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| activity?.SetTag("otel.status_code", "OK"); | ||||
| #endif | ||||
| return res2.Result; | ||||
| default: | ||||
| #if DEBUG | ||||
|
|
@@ -256,14 +289,25 @@ protected virtual T Rpc<T>(string callName, JToken parameters, JsonSerializer se | |||
| if (res1.Error != null) | ||||
| { | ||||
| var errorArray = res1.Error.ToObject<string[]>(); | ||||
| if (errorArray != null) { | ||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| activity?.SetTag("otel.status_code", "ERROR"); | ||||
|
There was a problem hiding this comment. I'd use |
||||
| //activity?.SetTag("rpc.jsonrpc.error_message", errorArray); | ||||
|
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. Should we be imlementing this properly rather than commenting it out - currently there is disparity between v1 and v2 implementation. |
||||
| #endif | ||||
| throw new Failure(errorArray); | ||||
| } | ||||
| } | ||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| activity?.SetTag("otel.status_code", "OK"); | ||||
| #endif | ||||
| return res1.Result; | ||||
| } | ||||
| } | ||||
| } | ||||
| } | ||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| } | ||||
| #endif | ||||
| } | ||||
| } | ||||
|
|
||||
|
|
@@ -293,6 +337,23 @@ protected virtual void PerformPostRequest(Stream postStream, Stream responseStre | |||
| webRequest.Headers.Add(header.Key, header.Value); | ||||
| } | ||||
|
|
||||
| #if (NET462_OR_GREATER || NETSTANDARD2_0_OR_GREATER) | ||||
| // 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) | ||||
|
|
||||
| var activity = Activity.Current; | ||||
| if (activity != null && activity.IdFormat == ActivityIdFormat.W3C) | ||||
| { | ||||
| webRequest.Headers.Add("traceparent", activity.Id); | ||||
| var state = activity.TraceStateString; | ||||
| if (state?.Length > 0) | ||||
| { | ||||
| webRequest.Headers.Add("tracestate", state); | ||||
| } | ||||
| } | ||||
| #endif | ||||
|
|
||||
| using (var str = webRequest.GetRequestStream()) | ||||
| { | ||||
| postStream.CopyTo(str); | ||||
|
|
||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| { | ||
| "version": 1, | ||
| "dependencies": { | ||
| ".NETFramework,Version=v4.5": { | ||
| "Microsoft.NETFramework.ReferenceAssemblies": { | ||
| "type": "Direct", | ||
| "requested": "[1.0.2, )", | ||
| "resolved": "1.0.2", | ||
| "contentHash": "5/cSEVld+px/CuRrbohO/djfg6++eR6zGpy88MgqloXvkj//WXWpFZyu/OpkXPN0u5m+dN/EVwLNYFUxD4h2+A==", | ||
| "dependencies": { | ||
| "Microsoft.NETFramework.ReferenceAssemblies.net45": "1.0.2" | ||
| } | ||
| }, | ||
| "Newtonsoft.Json": { | ||
| "type": "Direct", | ||
| "requested": "[13.0.3, )", | ||
| "resolved": "13.0.3", | ||
| "contentHash": "HrC5BXdl00IP9zeV+0Z848QWPAoCr9P3bDEZguI+gkLcBKAOxix/tLEAAHC+UvDNPv4a2d18lOReHMOagPa+zQ==" | ||
| }, | ||
| "Microsoft.NETFramework.ReferenceAssemblies.net45": { | ||
| "type": "Transitive", | ||
| "resolved": "1.0.2", | ||
| "contentHash": "Nm14pRmqB+4u2JEMdtngnbDcJidTmswMxOJ992TpTwiwcUTERxLlHwwSh0HiUoRjS0TO0sozsiB0h6FHjCUdEA==" | ||
| } | ||
| }, | ||
| ".NETStandard,Version=v2.0": { | ||
| "NETStandard.Library": { | ||
| "type": "Direct", | ||
| "requested": "[2.0.3, )", | ||
| "resolved": "2.0.3", | ||
| "contentHash": "st47PosZSHrjECdjeIzZQbzivYBJFv6P2nv4cj2ypdI204DO+vZ7l5raGMiX4eXMJ53RfOIg+/s4DHVZ54Nu2A==", | ||
| "dependencies": { | ||
| "Microsoft.NETCore.Platforms": "1.1.0" | ||
| } | ||
| }, | ||
| "Newtonsoft.Json": { | ||
| "type": "Direct", | ||
| "requested": "[13.0.3, )", | ||
| "resolved": "13.0.3", | ||
| "contentHash": "HrC5BXdl00IP9zeV+0Z848QWPAoCr9P3bDEZguI+gkLcBKAOxix/tLEAAHC+UvDNPv4a2d18lOReHMOagPa+zQ==" | ||
| }, | ||
| "System.Diagnostics.DiagnosticSource": { | ||
| "type": "Direct", | ||
| "requested": "[8.0.1, )", | ||
| "resolved": "8.0.1", | ||
| "contentHash": "vaoWjvkG1aenR2XdjaVivlCV9fADfgyhW5bZtXT23qaEea0lWiUljdQuze4E31vKM7ZWJaSUsbYIKE3rnzfZUg==", | ||
| "dependencies": { | ||
| "System.Memory": "4.5.5", | ||
| "System.Runtime.CompilerServices.Unsafe": "6.0.0" | ||
| } | ||
| }, | ||
| "Microsoft.NETCore.Platforms": { | ||
| "type": "Transitive", | ||
| "resolved": "1.1.0", | ||
| "contentHash": "kz0PEW2lhqygehI/d6XsPCQzD7ff7gUJaVGPVETX611eadGsA3A877GdSlU0LRVMCTH/+P3o2iDTak+S08V2+A==" | ||
| }, | ||
| "System.Buffers": { | ||
| "type": "Transitive", | ||
| "resolved": "4.5.1", | ||
| "contentHash": "Rw7ijyl1qqRS0YQD/WycNst8hUUMgrMH4FCn1nNm27M4VxchZ1js3fVjQaANHO5f3sN4isvP4a+Met9Y4YomAg==" | ||
| }, | ||
| "System.Memory": { | ||
| "type": "Transitive", | ||
| "resolved": "4.5.5", | ||
| "contentHash": "XIWiDvKPXaTveaB7HVganDlOCRoj03l+jrwNvcge/t8vhGYKvqV+dMv6G4SAX2NoNmN0wZfVPTAlFwZcZvVOUw==", | ||
| "dependencies": { | ||
| "System.Buffers": "4.5.1", | ||
| "System.Numerics.Vectors": "4.4.0", | ||
| "System.Runtime.CompilerServices.Unsafe": "4.5.3" | ||
| } | ||
| }, | ||
| "System.Numerics.Vectors": { | ||
| "type": "Transitive", | ||
| "resolved": "4.4.0", | ||
| "contentHash": "UiLzLW+Lw6HLed1Hcg+8jSRttrbuXv7DANVj0DkL9g6EnnzbL75EB7EWsw5uRbhxd/4YdG8li5XizGWepmG3PQ==" | ||
| }, | ||
| "System.Runtime.CompilerServices.Unsafe": { | ||
| "type": "Transitive", | ||
| "resolved": "6.0.0", | ||
| "contentHash": "/iUeP3tq1S0XdNNoMz5C9twLSrM/TH+qElHkXWaPvuNOt+99G75NrV0OS2EqHx5wMN7popYjpc8oTjC1y16DLg==" | ||
| } | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never merge source with TODOs on
master.Use the name of the package
XenServer.NETand 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 testingIf not, this needs a placeholder, and changes to pipelines. We can advise on that if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the client code uses the C# SDK as a project in their code,
typeof(JsonRpcClient).Assembly.GetName().Versionshould 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could do without the placeholder and just add a line in the README 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.