Set url.full attribute on spans logged for HTTP requests

[ahoppen]

We previously only set the request method on these spans, which made it hard to identify which exact HTTP request was causing this span to be emitted.

Also record the URL of the HTTP request to add more information to these spans. While at it, also record the request body size because we already had an attribute key configured for it.

[ahoppen]

Added the more scoped logging and configuration options to opt into full URL logging and to opt out to path logging.

I chose to mark DeconstructedURL as @usableFromInline so that I can use it for the structured field tracing. Let me know if there’s an issue with that.

[czechboy0]

I think url.full should be enabled by default, following OpenTelemetry standard attributes: https://opentelemetry.io/docs/specs/semconv/registry/attributes/url/#url-full

Almost any string field can, in theory, have PII, and I think this is the wrong place to try to make that call. Let application owners filter PII in their telemetry backends instead.

[ahoppen]

I think url.full should be enabled by default, following OpenTelemetry standard attributes

Are you suggesting that we don’t have any configuration options for this in async-http-client? I personally don’t have an opinion either way.

[czechboy0]

That's right. That's both how OpenTelemetry and the Swift observability libraries are designed.

[ahoppen]

Updated the PR to remove the configuration options.

[ktoso]

Just chiming in to say this looks good! Agree on not doing tens of settings in the lib itself, collectors can handle that 👍

[ahoppen]

@czechboy0 @FranzBusch could I get another review on this?

[ahoppen]

Updated to only strip username and password. I would appreciate another review.

[ktoso]

Looks good to me, we do our best but it’s collectors and vis config that may need to do more scrubbing anyway.

[glbrntt]

LGTM. I think the docs failure will be resolved by #913

[ahoppen]

If you’re happy with this, could you merge the PR? I don’t have merge access to this repo.

[glbrntt]

I'm going to wait for the other PR to be merged first as the failure was introduced by your code and we'd rather not merge new CI regressions.

[glbrntt]

Agh, sorry @ahoppen -- I was wrong, that change didn't fix it. It's not obvious to me why your change fails to build for docs but does for "regular" CI runs.

/__w/async-http-client/async-http-client/Sources/AsyncHTTPClient/TracingSupport.swift:40:13: error: switch must be exhaustive
 38 |     ) {
 39 |         let requestBodySize: Int? =
 40 |             switch request.body?.mode {
    |             |- error: switch must be exhaustive
    |             `- note: add missing case: '.some(.httpClientRequestBody(length: let length, startUpload: let startUpload))'
 41 |             case .asyncSequence(.known(let length), _), .sequence(.known(let length), _, _):
 42 |                 Int(length)

[ahoppen]

Oh, interesting that this got caught as part of the doc check. I just didn’t check that code builds with UnstableHTTPAPIsSupport enabled. Should be fixed now.