Conform client's tracing interceptor to OTel's conventions #25

gjcairo · 2025-01-16T16:55:57Z

This PR conforms the client tracing interceptor to follow recommendations/conventions laid out in both:

gjcairo · 2025-01-16T17:08:27Z

This API break is expected, as the interceptor has been renamed:

💔 API breakage: struct ClientTracingInterceptor has been removed

Sources/GRPCInterceptors/Tracing/ClientOTelTracingInterceptor.swift

Tests/GRPCInterceptorsTests/TracingInterceptorTests.swift

Sources/GRPCInterceptors/HookedAsyncSequence.swift

Sources/GRPCInterceptors/Tracing/ClientOTelTracingInterceptor.swift

Sources/GRPCInterceptors/HookedAsyncSequence.swift

Sources/GRPCInterceptors/Tracing/ClientOTelTracingInterceptor.swift

Sources/GRPCInterceptors/Tracing/SpanAttributes+RPCAttributes.swift

glbrntt · 2025-01-21T08:57:35Z

Sources/GRPCInterceptors/Tracing/SpanAttributes+RPCAttributes.swift

+    case .other(let address):
+      // We can't nicely format the span attributes to contain the appropriate information here,
+      // so include the whole thing as part of the server address.
+      self.attributes.rpc.serverAddress = address


I'm not sure about this. We set serverAddress above to serverHostname which is a value explicitly passed in by the user so I don't think we should be overriding it here.

I think my lean is to ignore the remote peer if we can't parse it. (Meaning PeerAddress should have a failable init rather than other.)

Actually this is a mistake - I think we should be setting network.peer.address. I agree the server address should stay as the hostname. The port is only required if the transport supports it, which we don't know at this stage so we can just leave it blank.

This one didn't get fixed.

Yeah I've changed it to set "network.peer.address" instead

glbrntt · 2025-01-21T09:05:10Z

Sources/GRPCInterceptors/Tracing/SpanAttributes+RPCAttributes.swift

+      self.attributes.rpc.serverPort = port
+
+    case .unixDomainSocket(let path):
+      self.attributes.rpc.networkType = "unix"


Looking at the docs https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/, it seems like network type should be either "ipv4"/"ipv6" and that "unix" should be a transport.

Hm, so should we leave the network type empty when using UDS?

Also unsure about setting the network transport here, since we're getting it as an argument when initialising the interceptor.

network.type has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

We don't have a well-known value here (they list "ipv4" and "ipv6") only. We could use a custom value but not sure that would actually add useful information (given we'd set "network.transport" to "unix") so I think we shouldn't set it.

Also unsure about setting the network transport here, since we're getting it as an argument when initialising the interceptor.

What about this? IMO, we shouldn't set it, as it's the same argument as we don't want to override the server address; we don't want to override the network transport either.

Sorry, forgot to reply to that one. I agree we shouldn't set it; let's leave that one up to the user.

Sources/GRPCInterceptors/Tracing/SpanAttributes+RPCAttributes.swift

glbrntt

I think a few things got missed but this is definitely much closer to where it needs to be!

Tests/GRPCInterceptorsTests/PeerAddressTests.swift

glbrntt · 2025-01-21T14:21:03Z

Sources/GRPCInterceptors/Tracing/SpanAttributes+RPCAttributes.swift

+      self.attributes.rpc.serverPort = port
+
+    case .unixDomainSocket(let path):
+      self.attributes.rpc.networkType = "unix"


network.type has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.

We don't have a well-known value here (they list "ipv4" and "ipv6") only. We could use a custom value but not sure that would actually add useful information (given we'd set "network.transport" to "unix") so I think we shouldn't set it.

glbrntt · 2025-01-21T14:23:49Z

Sources/GRPCInterceptors/Tracing/SpanAttributes+RPCAttributes.swift

+    case .other(let address):
+      // We can't nicely format the span attributes to contain the appropriate information here,
+      // so include the whole thing as part of the server address.
+      self.attributes.rpc.serverAddress = address


This one didn't get fixed.

Sources/GRPCInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift

glbrntt · 2025-01-21T18:01:27Z

Sources/GRPCInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift

+      guard addressComponents.count == 3, let port = Int(addressComponents[2]) else {
+        // This is some unexpected/unknown format, so we have no way of splitting it up nicely.
+        self = .other(address)
+        return
+      }
+      self = .ipv4(address: String(addressComponents[1]), port: port)


A previous comment got missed: #25 (comment)

This is also fixed with the changes I brought from the server PR

Sources/GRPCInterceptors/Tracing/SpanAttributes+RPCAttributes.swift

glbrntt · 2025-01-21T18:04:14Z

Tests/GRPCInterceptorsTests/PeerAddressTests.swift

+  @Test("Unrecognised addresses are correctly parsed")
+  func testOther() {
+    let address = PeerAddress("in-process:1234")
+    #expect(address == .other("in-process:1234"))
+  }


I'm still not sure whether we should include addresses we can't parse. I think my lean is to not include them because we're then more likely to catch parsing bugs (I think a user is more likely to report an issue if the information is missing rather than not quite appearing as it should).

Hm, I'm not sure about this. I feel like silently skipping information is worse than showing it in a not-ideal way.

I'm not sure I agree: we don't know what the value is so we should treat it with a bit of suspicion.

I don't feel that strongly about this so I don't mind not setting it, but what is the worst that could happen if we did set it?

I can think of a few things:

If, for some reason, the string is very dynamic (i.e. changes for each RPC) then we could pollute the users telemetry collection service with thousands of potentially useless values.

A very long could cause issues if the collector doesn't handle long strings well.

In practice these both seem pretty unlikely, but they are possible.

IMO the more compelling reason to not set it is that users are more likely to complain if the value is missing and then we can figure out why parsing failed so we can set it correctly.

I also find it weird that we can return .other when we know it's ipv4/ipv6 but we can't parse it, because it doesn't meet our expectations of what that address type should be.

glbrntt

This basically looks good; just the one nit around "[" and resolving how to handle that.

glbrntt · 2025-01-22T15:39:28Z

Sources/GRPCInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift

+      // At this point, we are looking at an address with format: [<address>]:<port>
+      // We drop the first character ('[') and split by ']:' to keep two components: the address
+      // and the port.
+      let ipv6AddressComponents = addressComponents[1].dropFirst().split(separator: "]:")


I think we should be a bit stricter here and check that we actually have a "[".

We also have plenty of allocations here which are mostly avoidable (which I'm fine to punt on for now), but could you file an issue? The idea would be to avoid split and all the extra String conversions by using the UTF8 view.

glbrntt · 2025-01-22T15:53:21Z

Tests/GRPCInterceptorsTests/PeerAddressTests.swift

+  @Test("Unrecognised addresses are correctly parsed")
+  func testOther() {
+    let address = PeerAddress("in-process:1234")
+    #expect(address == .other("in-process:1234"))
+  }


I can think of a few things:

If, for some reason, the string is very dynamic (i.e. changes for each RPC) then we could pollute the users telemetry collection service with thousands of potentially useless values.

A very long could cause issues if the collector doesn't handle long strings well.

In practice these both seem pretty unlikely, but they are possible.

IMO the more compelling reason to not set it is that users are more likely to complain if the value is missing and then we can figure out why parsing failed so we can set it correctly.

I also find it weird that we can return .other when we know it's ipv4/ipv6 but we can't parse it, because it doesn't meet our expectations of what that address type should be.

This PR brings the same changes made to the client in #25 to the server interceptor, conforming it to follow recommendations/conventions laid out in both: - https://opentelemetry.io/docs/specs/semconv/rpc/rpc-spans - https://opentelemetry.io/docs/specs/semconv/rpc/grpc/ --------- Co-authored-by: George Barnett <[email protected]>

gjcairo added the 🆕 semver/minor Adds new public API. label Jan 16, 2025

gjcairo requested a review from glbrntt January 16, 2025 16:55

gjcairo added ⚠️ semver/major Breaks existing public API. and removed 🆕 semver/minor Adds new public API. labels Jan 16, 2025

gjcairo changed the title ~~Conform tracing interceptor to OTel's conventions~~ Conform client's tracing interceptor to OTel's conventions Jan 17, 2025

gjcairo mentioned this pull request Jan 17, 2025

Conform server tracing interceptor to OTel's conventions #27

Merged

gjcairo force-pushed the otel-client-interceptor branch from 1d1f018 to 2929500 Compare January 17, 2025 17:57

glbrntt requested changes Jan 20, 2025

View reviewed changes

glbrntt reviewed Jan 20, 2025

View reviewed changes

gjcairo added 6 commits January 20, 2025 16:13

Fix license check script

02c3cbb

Add OTel attributes and rename to ClienOTelTracingInterceptor

424b8aa

Small changes to server interceptor

7cada1d

Update tests

ab38ee3

Formatting

7a95594

Handle response failures + do not record custom events

dd7bc23

gjcairo force-pushed the otel-client-interceptor branch from 48530eb to dd7bc23 Compare January 20, 2025 16:14

gjcairo added 5 commits January 20, 2025 16:23

Add PeerAddress and refactor span attributes

a2adc5e

Formatting

4c11858

Parametrise tests and move to swift-testing

032c0b3

Formatting

4d24cd9

Add new test

34bbf9f

gjcairo requested a review from glbrntt January 20, 2025 18:02

glbrntt requested changes Jan 21, 2025

View reviewed changes

PR changes

01186b1

gjcairo requested a review from glbrntt January 21, 2025 11:12

glbrntt requested changes Jan 21, 2025

View reviewed changes

gjcairo requested a review from glbrntt January 21, 2025 19:59

Bring over PeerAddress parsing changes

3275f72

gjcairo force-pushed the otel-client-interceptor branch from 41aa5fe to 3275f72 Compare January 21, 2025 20:00

glbrntt requested changes Jan 22, 2025

View reviewed changes

PR changes

10189c7

gjcairo requested a review from glbrntt January 22, 2025 16:37

glbrntt approved these changes Jan 23, 2025

View reviewed changes

Fix test

324a6ee

glbrntt enabled auto-merge (squash) January 23, 2025 13:36

Depend on grpc-swift-protobuf main branch

e160b26

glbrntt disabled auto-merge January 23, 2025 13:45

glbrntt merged commit ee4936a into grpc:main Jan 23, 2025
20 of 21 checks passed

gjcairo deleted the otel-client-interceptor branch January 23, 2025 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conform client's tracing interceptor to OTel's conventions #25

Conform client's tracing interceptor to OTel's conventions #25

gjcairo commented Jan 16, 2025

gjcairo commented Jan 16, 2025

glbrntt Jan 21, 2025

gjcairo Jan 21, 2025

glbrntt Jan 21, 2025

gjcairo Jan 21, 2025

glbrntt Jan 21, 2025

gjcairo Jan 21, 2025

gjcairo Jan 21, 2025

glbrntt Jan 21, 2025

gjcairo Jan 21, 2025 •

edited

Loading

glbrntt Jan 22, 2025

glbrntt left a comment

glbrntt Jan 21, 2025

glbrntt Jan 21, 2025

glbrntt Jan 21, 2025

gjcairo Jan 21, 2025 •

edited

Loading

glbrntt Jan 21, 2025

gjcairo Jan 21, 2025

glbrntt Jan 22, 2025

gjcairo Jan 22, 2025

glbrntt Jan 22, 2025

glbrntt left a comment

glbrntt Jan 22, 2025

gjcairo Jan 22, 2025

glbrntt Jan 22, 2025

Conform client's tracing interceptor to OTel's conventions #25

Conform client's tracing interceptor to OTel's conventions #25

Conversation

gjcairo commented Jan 16, 2025

gjcairo commented Jan 16, 2025

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

gjcairo Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt 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

gjcairo Jan 21, 2025 • 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

glbrntt 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

gjcairo Jan 21, 2025 •

edited

Loading

gjcairo Jan 21, 2025 •

edited

Loading