-
Notifications
You must be signed in to change notification settings - Fork 143
Add more span attributes (URL, network) #881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
6dc5dfa
f096a44
52ee55b
16b4984
c580487
25dfc39
762f1c8
c64e162
fbb95a4
fe63763
4d287d8
26ff5f5
8263ad1
17ed657
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import NIOHTTP1 | ||
| import OTelSemanticConventions | ||
| import Tracing | ||
|
|
||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | ||
|
|
@@ -27,13 +28,32 @@ extension HTTPClient { | |
| } | ||
|
|
||
| return try await tracer.withSpan(request.method.rawValue, ofKind: .client) { span in | ||
| let keys = self.configuration.tracing.attributeKeys | ||
| span.attributes[keys.requestMethod] = request.method.rawValue | ||
| // TODO: set more attributes on the span | ||
| span.attributes.http.request.method = .init(rawValue: request.method.rawValue) | ||
|
|
||
| // set request headers | ||
| for header in request.headers { | ||
| span.attributes.http.request.header.set(header.name, to: [header.value]) | ||
| } | ||
|
|
||
| // set url attributes | ||
| if let deconstructedURL = try? DeconstructedURL(url: request.url) { | ||
| span.attributes.url.path = deconstructedURL.uri | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The query (part of the path, I believe here?) could also include a secret token, but it's tricky to try to filter it out as it can have any name.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have changed the implementation to redact query names based on a list inside OTel says: So I have added them by default to the redaction list, with the possibility to extend it upon creating a client. |
||
| span.attributes.url.scheme = deconstructedURL.scheme.rawValue | ||
| span.attributes.server.address = deconstructedURL.connectionTarget.host | ||
| span.attributes.server.port = deconstructedURL.connectionTarget.port | ||
| } | ||
|
|
||
| let response = try await body() | ||
|
|
||
| // set response span attributes | ||
| TracingSupport.handleResponseStatusCode(span, response.status, keys: tracing.attributeKeys) | ||
| TracingSupport.handleResponseStatusCode(span, response.status) | ||
|
|
||
| for header in response.headers { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as on the request side, I wonder if we should redact or remove
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Response headers are treated now the same way as request headers and are filtered through an allow list. I wasn't sure whether keeping two separate (one for request, other for response) would be beneficial, so I stayed with one |
||
| span.attributes.http.response.header.set(header.name, to: [header.value]) | ||
| } | ||
|
|
||
| // set network protocol version | ||
| span.attributes.network.protocol.version = "\(response.version.major).\(response.version.minor)" | ||
|
|
||
| return response | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1089,33 +1089,12 @@ public final class HTTPClient: Sendable { | |
| } | ||
| } | ||
|
|
||
| // TODO: Open up customization of keys we use? | ||
| /// Configuration for tracing attributes set by the HTTPClient. | ||
| @usableFromInline | ||
| package var attributeKeys: AttributeKeys | ||
|
|
||
| public init() { | ||
| if #available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) { | ||
| self._tracer = InstrumentationSystem.tracer | ||
| } else { | ||
| self._tracer = nil | ||
| } | ||
| self.attributeKeys = .init() | ||
| } | ||
|
|
||
| /// Span attribute keys that the HTTPClient should set automatically. | ||
| /// This struct allows the configuration of the attribute names (keys) which will be used for the apropriate values. | ||
| @usableFromInline | ||
| package struct AttributeKeys: Sendable { | ||
| @usableFromInline package var requestMethod: String = "http.request.method" | ||
| @usableFromInline package var requestBodySize: String = "http.request.body.size" | ||
|
|
||
| @usableFromInline package var responseBodySize: String = "http.response.body.size" | ||
| @usableFromInline package var responseStatusCode: String = "http.status_code" | ||
|
|
||
| @usableFromInline package var httpFlavor: String = "http.flavor" | ||
|
|
||
| @usableFromInline package init() {} | ||
|
Comment on lines
-1130
to
-1142
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of those options and their use being removed. The goal here is to allow users to change the attribute keys. |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import NIOConcurrencyHelpers | |
| import NIOCore | ||
| import NIOHTTP1 | ||
| import NIOSSL | ||
| import OTelSemanticConventions | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of adding another package dependency just to import some Strings. As the OTEL strings should never change, we can just have those strings hardcoded here in this package.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the dependency and brought back the keys |
||
| import Tracing | ||
|
|
||
| // MARK: - Centralized span attribute handling | ||
|
|
@@ -26,13 +27,13 @@ struct TracingSupport { | |
| @inlinable | ||
| static func handleResponseStatusCode( | ||
| _ span: Span, | ||
| _ status: HTTPResponseStatus, | ||
| keys: HTTPClient.TracingConfiguration.AttributeKeys | ||
| _ status: HTTPResponseStatus | ||
| ) { | ||
| if status.code >= 400 { | ||
| span.setStatus(.init(code: .error)) | ||
| } | ||
| span.attributes[keys.responseStatusCode] = SpanAttribute.int64(Int64(status.code)) | ||
|
|
||
| span.attributes.http.response.statusCode = Int(status.code) | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Does OTel recommend redacting any headers? For example, should this include
Authorization/Cookieheaders?Uh oh!
There was an error while loading. Please reload this page.
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 and thanks for the question. It seems like OTel does not recommend capturing anything by default
I have changed the implementation to an opt-in array residing inside TracingConfiguration.
One thing I am unsure about is how these headers would be configured in frameworks like Vapor for instance where a default client is supplied in the
Requestobject. Maybe in that case the configuration would be easier once swift-configuration is adopted? For newly instantiated clients that should be easier, though.