Instrument AWSClient WIP #348
Conversation
Codecov Report
@@ Coverage Diff @@
## main #348 +/- ##
==========================================
+ Coverage 74.09% 74.34% +0.25%
==========================================
Files 60 61 +1
Lines 4439 4545 +106
==========================================
+ Hits 3289 3379 +90
- Misses 1150 1166 +16
Continue to review full report at Codecov.
|
| fileprivate func invoke(_ httpRequest: AWSHTTPRequest, with serviceConfig: AWSServiceConfig, on eventLoop: EventLoop, logger: Logger, context: Context) -> EventLoopFuture<AWSHTTPResponse> { | ||
| // TODO: what should be the operation name? | ||
| let operationName: String = httpRequest.url.path | ||
| var span = InstrumentationSystem.tracingInstrument.startSpan(named: operationName, context: context, ofKind: .client, at: .now()) |
There was a problem hiding this comment.
There was a long conversation for the Logger PR in async-http-client swift-server/async-http-client#227 about messages and how they should not include context information but instead the metadata should hold the context. @ktoso was heavily involved in this. I am assuming this should be the case here as well. I've already mentioned elsewhere about adding context keys to the baggage. So the operation name should be something consistent like "invoke".
There was a problem hiding this comment.
the metadata should hold the context.
the baggage contains the metadata with the context
my understanding is that the current approach is that we should update baggage and use it to set logger metadata,
see also discussion here swift-server/swift-aws-lambda-runtime#162 (comment)
that way it should not be needed to explicitly change the logger (although as of now LoggingBaggageContextCarrier requires logger to be mutable ...),
also we would also have baggage and logger synced
may be affected by slashmo/gsoc-swift-baggage-context#23 (comment)
same as https://github.com/swift-server/async-http-client/pull/289/files#r478461213
There was a problem hiding this comment.
You aren't adding any metadata to the baggage though.
I also added this slashmo/gsoc-swift-baggage-context#28. There is no guarantee the baggage is being added to the Logger metadata. You have to trust the implementation of the context object
There was a problem hiding this comment.
You aren't adding any metadata to the baggage though.
logger.attachingRequestId(Self.globalRequestID.add(1), operation: operationName, service: serviceConfig.service)you are right as far as requestId, operationName and service are concerned
the trace context is added, what exactly is added depends on implementation
typically that will include traceId but not requestId (which, as far as XRay is concerned, is span/segment attribute)
this is the same as https://github.com/swift-server/async-http-client/pull/289/files#r478461213
There was a problem hiding this comment.
how about sth like:
private struct RequestMetadata: CustomStringConvertible {
var requestId: Int
var service: String
var operation: String
var description: String {
"aws-request-id=\(requestId) aws-service=\(service) aws-operation=\(operation)"
}
}
private enum RequestKey: BaggageContextKey {
typealias Value = RequestMetadata
var name: String { "AWSRequest" }
}then updating the baggage like:
context.baggage[RequestKey.self] = RequestMetadata(requestId: Self.globalRequestID.add(1),
service: serviceConfig.service,
operation: "signURL")instead of updating the logger directly (current):
context.logger = context.logger.attachingRequestId(Self.globalRequestID.add(1), operation: "signURL", service: serviceConfig.service)There was a problem hiding this comment.
What does the Logger output look like?
There was a problem hiding this comment.
Ignore that saw your comment above
There was a problem hiding this comment.
When it comes to logger formatting I leave that to @ktoso he has stronger views on this I think :-)
There was a problem hiding this comment.
It could be just that NoOpTracingInstrument and it's span are wrong, I'll have to consult OTel but it seems reasonable that we should always carry and the Noop is simply implemented wrong/lazy. I did not have much time to work on this in the last week, let's hope next weeks will change that.
There was a problem hiding this comment.
fixed with slashmo/gsoc-swift-tracing#138
You mean baggage and tracing? Yes, they have to remain separate to avoid weird cyclic dependency issues. Some libs will not want to directly adopt swift-tracing but they will carry baggage context, but nothing more (i.e. not start spans etc) |
|
Comments in code. output of AWSXRaySDKExampleAWS note that the segments are started when future is created, which does not look nice, hmm // TODO: segments are started when future is created
let listBucketObjects = buckets.map { listObjects(bucket: $0, context: segmentContext) }
return EventLoopFuture.andAllComplete(listBucketObjects, on: eventLoop)
some of the attributes recorded by AHC (note that XRay attributes should not have "." so they are removed, this should be mapped to XRay own http object at some point after
|
|
|
||
| // TODO: extensions based on XRaySDK API, temporary, see https://github.com/slashmo/gsoc-swift-tracing/issues/125 | ||
|
|
||
| private extension TracingInstrument { |
There was a problem hiding this comment.
Is this temporary until there are changes elsewhere? If not can we move to another file
There was a problem hiding this comment.
will do, this code will be redundant if the helpers are created per slashmo/gsoc-swift-tracing#125
I dont think its high priority so this may take a while or maybe will never happen
will move to separate file
| return try awsRequest | ||
| .applyMiddlewares(serviceConfig.middlewares + self.middlewares) | ||
| .applyMiddlewares(config.middlewares + self.middlewares) | ||
| .createHTTPRequest(signer: signer) |
There was a problem hiding this comment.
The majority of time spent in execute outside of the HTTP request is the signing of the request. This happens inside createHTTPRequest. There currently isn't a span around this.
There was a problem hiding this comment.
creating spans for signing and middleware
| func getCredential(on eventLoop: EventLoop, logger: Logger) -> EventLoopFuture<Credential> | ||
| typealias Context = BaggageLogging.LoggingBaggageContextCarrier | ||
|
|
||
| func getCredential(on eventLoop: EventLoop, context: CredentialProvider.Context) -> EventLoopFuture<Credential> |
There was a problem hiding this comment.
I see you have replaced Logger with Context but aren't using it yet in the credential providers code. If you were to start the tracing of the credential providers. I would look at DeferredCredentialProvider.init, RotatingCredentialProvider.refreshCredentials and RuntimeSelectorCredentialProvider.setupInternalProvider. These generally wrap all the other providers.
|
I assume you have rebuilt all the |
I have not, no idea how to do it (yet) :-) I just copied one service and updated them using I dont like that the order of context and eventLoop is different in different places (depending on if eventLoop is optional or not); otherwise I think if services generation is well documented I can try to do it, but I am done with coding this week so this would wait till sometime next week thank you for the comments, I will get thought them and try to address the issues next week as well |
Yeah the sensitivity of that param depending on that is a bit annoying... could you please provide an example of a few methods like that as a ticket on baggage context? Does it show up a lot? |
Where is the inconsistency. I thought EventLoop was optional in all the api calls. Maybe I missed one |
Assuming you are in aws-sdk-swift folder Have a good weekend |
|
I was a bit busy last week. Now, this is not the final version and there are places for improvements and fixes. I think the main objectives have been:
I think the next steps are:
My guess it will take 3 months (1 month for "cleanups", 1 for SSWG approval, 1 for "unplanned delay"),
@adam-fowler I think you need to decide if the proposed approach makes sense and could be adopted. if you approve the use public protocol AWSClientContext {
var logger: Logger { get }
}
extension AWSClient {
public typealias Context = AWSClientContext
}which then could be extended to extension AWSClient {
public typealias Context = BaggageLogging.LoggingBaggageContextCarrier
}just an idea |
most public function do not require eventLoop (its optional): public func copyObject(_ input: CopyObjectRequest, context: AWSClient.Context, on eventLoop: EventLoop? = nil) -> EventLoopFuture<CopyObjectOutput>some do, in which case the order of public func multipartDownload(
_ input: GetObjectRequest,
partSize: Int = 5 * 1024 * 1024,
on eventLoop: EventLoop,
context: AWSClient.Context,
outputStream: @escaping (ByteBuffer, Int64, EventLoop) -> EventLoopFuture<Void>
) |
That should probably be an optional |
|
Sure, this one is outdated anyway. I dont expect you will ship it until AHC supports tracing, which has not happened yet https://github.com/soto-project/soto-core/pull/443/files#r641917520 |
Early PoC of AWS Client tracing instrumentation.