Add more span attributes (URL, network) #881
Conversation
Motivation: Adding more trace span attributes in order to be able to intrument any system using AsyncHTTPClient, better. Modifications: Exposed DeconstructedURL to be usable from inline to reach into specific url components. Result: The span attributes `http.response.body.size`, `http.response.status_code`, `network.protocol.version`, `url.path`, `url.scheme`, `server.hostname` and `server.port` should be available on the span.
| @usableFromInline package var urlScheme: String = "url.scheme" | ||
|
|
||
| @usableFromInline package var serverHostname: String = "server.hostname" | ||
| @usableFromInline package var serverPort: String = "server.port" |
There was a problem hiding this comment.
I wonder if we should just take the direct dependency on https://github.com/swift-otel/swift-otel-semantic-conventions
There was a problem hiding this comment.
This sounds like a great addition - I've added it. Please let me know (you and the maintainers) if the implementation is fine.
I've also added request and response header attributes to the span.
czechboy0
left a comment
czechboy0
left a comment
There was a problem hiding this comment.
Added a few suggestions, but otherwise it's great to see more attributes added here.
| span.attributes.http.request.method = .init(rawValue: request.method.rawValue) | ||
|
|
||
| // set request headers | ||
| for header in request.headers { |
There was a problem hiding this comment.
Does OTel recommend redacting any headers? For example, should this include Authorization/Cookie headers?
There was a problem hiding this comment.
Yes and thanks for the question. It seems like OTel does not recommend capturing anything by default
Instrumentations SHOULD require an explicit configuration of which headers are to be captured. Including all request headers can be a security risk - explicit configuration helps avoid leaking sensitive information.
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 Request object. Maybe in that case the configuration would be easier once swift-configuration is adopted? For newly instantiated clients that should be easier, though.
|
|
||
| // set url attributes | ||
| if let deconstructedURL = try? DeconstructedURL(url: request.url) { | ||
| span.attributes.url.path = deconstructedURL.uri |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have changed the implementation to redact query names based on a list inside TracingConfiguration.
OTel says:
Query string values for the following keys SHOULD be redacted by default and replaced by the value REDACTED:
[AWSAccessKeyId](https://docs.aws.amazon.com/AmazonS3/latest/userguide/RESTAuthentication.html#RESTAuthenticationQueryStringAuth)
[Signature](https://docs.aws.amazon.com/AmazonS3/latest/userguide/RESTAuthentication.html#RESTAuthenticationQueryStringAuth)
[sig](https://learn.microsoft.com/azure/storage/common/storage-sas-overview#sas-token)
[X-Goog-Signature](https://cloud.google.com/storage/docs/access-control/signed-urls)
So I have added them by default to the redaction list, with the possibility to extend it upon creating a client.
| 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.
Same question as on the request side, I wonder if we should redact or remove Set-Cookie here.
There was a problem hiding this comment.
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
| @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.
Allowed headers is one way, though I wonder if it should be "redacted headers" instead, similar to the path and query items below.
2 participants
This PR adds some span attributes mentioned in #860
http.response.body.sizehttp.status_code->http.response.status_codenetwork.protocol.versionurl.pathurl.schemeserver.hostnameserver.porthttp.request.headerhttp.response.header