-
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 5 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 |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -1089,33 +1098,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 |
|---|---|---|
|
|
@@ -12,11 +12,13 @@ | |
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| 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 | ||
| 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 +28,35 @@ 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) | ||
| } | ||
|
|
||
| @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: "&") | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
See comment above, re making this less computational heavy.