-
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 7 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 |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
|
|
||
| import NIOHTTP1 | ||
| import Tracing | ||
| import struct Foundation.URL | ||
|
|
||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | ||
| extension HTTPClient { | ||
|
|
@@ -29,12 +30,64 @@ 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 | ||
|
|
||
| // set explicitly allowed request headers | ||
| let allowedRequestHeaderNames = Set(request.headers.map(\.name)).intersection(configuration.tracing.allowedHeaders) | ||
|
|
||
| for headerName in allowedRequestHeaderNames { | ||
| let values = request.headers[headerName] | ||
|
|
||
| if !values.isEmpty { | ||
| span.attributes["\(keys.requestHeader).\(headerName)"] = values | ||
| } | ||
| } | ||
|
|
||
| // set url attributes | ||
| if let url = URL(string: request.url) { | ||
| span.attributes[keys.urlPath] = TracingSupport.sanitizePath( | ||
| url.path, | ||
| redactionComponents: self.configuration.tracing.sensitivePathComponents | ||
| ) | ||
|
|
||
| if let scheme = url.scheme { | ||
| span.attributes[keys.urlScheme] = scheme | ||
| } | ||
| if let query = url.query { | ||
| span.attributes[keys.urlQuery] = TracingSupport.sanitizeQuery( | ||
| query, | ||
| redactionComponents: self.configuration.tracing.sensitiveQueryComponents | ||
| ) | ||
| } | ||
| if let fragment = url.fragment { | ||
| span.attributes[keys.urlFragment] = fragment | ||
| } | ||
| if let host = url.host { | ||
| span.attributes[keys.serverHostname] = host | ||
| } | ||
| if let port = url.port { | ||
| span.attributes[keys.serverPort] = port | ||
| } | ||
| } | ||
|
|
||
| let response = try await body() | ||
|
|
||
| // set response span attributes | ||
| TracingSupport.handleResponseStatusCode(span, response.status, keys: tracing.attributeKeys) | ||
|
|
||
| // set explicitly allowed response headers | ||
| let allowedResponseHeaderNames = Set(response.headers.map(\.name)).intersection(configuration.tracing.allowedHeaders) | ||
|
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. See comment above, re making this less computational heavy. |
||
|
|
||
| for headerName in allowedResponseHeaderNames { | ||
| let values = response.headers[headerName] | ||
|
|
||
| if !values.isEmpty { | ||
| span.attributes["\(keys.responseHeader).\(headerName)"] = values | ||
| } | ||
| } | ||
|
|
||
| // set network protocol version | ||
| span.attributes[keys.networkProtocolVersion] = "\(response.version.major).\(response.version.minor)" | ||
|
|
||
| return response | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1070,6 +1070,15 @@ public final class HTTPClient: Sendable { | |
| @usableFromInline | ||
| var _tracer: Optional<any Sendable> // erasure trick so we don't have to make Configuration @available | ||
|
|
||
| public var allowedHeaders: Set<String> = [] | ||
|
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. Allowed headers is one way, though I wonder if it should be "redacted headers" instead, similar to the path and query items below.
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. This would work well when it comes to request headers. But I'm thinking if for the cases like the response headers, where we're sometimes communicating with an external services that we have no control over, whether this wouldn't lead to leaking some secrets of custom names, either at the current time, or in case that service evolves and starts sending other headers in response. Maybe in that case ignoring everything unless explicitly allowed would be safer, what do you think? |
||
| public var sensitivePathComponents: Set<String> = [] | ||
| public var sensitiveQueryComponents: Set<String> = [ | ||
| "AWSAccessKeyId", | ||
| "Signature", | ||
| "sig", | ||
| "X-Goog-Signature" | ||
| ] | ||
|
|
||
| /// Tracer that should be used by the HTTPClient. | ||
| /// | ||
| /// This is selected at configuration creation time, and if no tracer is passed explicitly, | ||
|
|
@@ -1109,12 +1118,22 @@ public final class HTTPClient: Sendable { | |
| package struct AttributeKeys: Sendable { | ||
| @usableFromInline package var requestMethod: String = "http.request.method" | ||
| @usableFromInline package var requestBodySize: String = "http.request.body.size" | ||
|
|
||
| @usableFromInline package var requestHeader: String = "http.request.header" | ||
| @usableFromInline package var responseHeader: String = "http.response.header" | ||
| @usableFromInline package var responseBodySize: String = "http.response.body.size" | ||
| @usableFromInline package var responseStatusCode: String = "http.status_code" | ||
|
|
||
| @usableFromInline package var responseStatusCode: String = "http.response.status_code" | ||
| @usableFromInline package var httpFlavor: String = "http.flavor" | ||
|
|
||
| @usableFromInline package var networkProtocolVersion: String = "network.protocol.version" | ||
|
|
||
| @usableFromInline package var urlPath: String = "url.path" | ||
| @usableFromInline package var urlScheme: String = "url.scheme" | ||
| @usableFromInline package var urlQuery: String = "url.query" | ||
| @usableFromInline package var urlFragment: String = "url.fragment" | ||
|
|
||
| @usableFromInline package var serverHostname: String = "server.hostname" | ||
| @usableFromInline package var serverPort: String = "server.port" | ||
|
fabianfett marked this conversation as resolved.
|
||
|
|
||
| @usableFromInline package init() {} | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import Foundation | ||
|
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 think we had a change so that we only rely on FoundationEssentials. |
||
| import Logging | ||
| import NIOConcurrencyHelpers | ||
| import NIOCore | ||
|
|
@@ -32,8 +33,31 @@ struct TracingSupport { | |
| if status.code >= 400 { | ||
| span.setStatus(.init(code: .error)) | ||
| } | ||
|
|
||
| span.attributes[keys.responseStatusCode] = SpanAttribute.int64(Int64(status.code)) | ||
| } | ||
|
|
||
| @inlinable | ||
| static func sanitizePath(_ path: String, redactionComponents: Set<String>) -> String { | ||
| redactionComponents.reduce(path) { path, component in | ||
| path.replacingOccurrences(of: component, with: "REDACTED") | ||
| } | ||
| } | ||
|
|
||
| @inlinable | ||
| static func sanitizeQuery(_ query: String, redactionComponents: Set<String>) -> String { | ||
| query.components(separatedBy: "&").map { | ||
| let nameAndValue = $0 | ||
| .trimmingCharacters(in: .whitespaces) | ||
| .components(separatedBy: "=") | ||
|
|
||
| if redactionComponents.contains(nameAndValue[0]) { | ||
| return "\(nameAndValue[0])=REDACTED" | ||
| } | ||
|
|
||
| return $0 | ||
| }.joined(separator: "&") | ||
| } | ||
| } | ||
|
|
||
| // MARK: - HTTPHeadersInjector | ||
|
|
||
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.
That is quite expensive. We should reduce this down to a loop and a O(1) lookup in the
allowedHeaderslist.