Add OTEL URL attributes

[NeedleInAJayStack]

Motivation:

Aligning with current OTEL semantic span attributes. URL spans attributes are required by current HTTP server semantic standards: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-server-semantic-conventions

Modifications:

Adds OTEL URL span attributes

Result:

Increases public API to include OTEL URL span attributes

[NeedleInAJayStack]

I added the current OTEL URL semantic standard, but did notice that the package currently links to OTEL v1.11.0, and some definitions have become out of date.

Just let me know if you'd prefer that I align with v1.11.0, or what the plan is for updating to newer versions.

Thanks!

[slashmo]

Just let me know if you'd prefer that I align with v1.11.0, or what the plan is for updating to newer versions.

Thanks for bringing this up 🙏 @ktoso How do we want to go forward with the OTel semantics library in general? IIRC, we discussed it doesn't make sense to tag semantic releases of this repo, but how about updating to align with newer OTel semantic conventions?

[ktoso]

Hi folks!

Okey so the intent of "extras" originally was "things that are not core to tracing and also are not stable" etc.

Today the world has moved on a bit and semantic conventions are stable and versioned: https://opentelemetry.io/docs/specs/semconv/

I would suggest we rename this -extras package into "swift-distributed-tracing-otel-semantic-conventions" and have a target per convention group (like HTTP).

We can version and follow along otel specifically then; i.e. the package could literarily have the same version as the semantic conventions it models.

Extras we try to steer away from (I know @czechboy0 who's likely to help out here as well), also would be happy if we don't have extras repos like that.

Maybe we can include not stable ones as well and just _ prefix them etc.

We need a moment to figure out the renames etc but I think this would be good.

[czechboy0]

I'm also curious what @slashmo would think about this becoming non-tracing specific, to be able to include e.g. metrics OTel conventions (we could rename accordingly, since this is pre-1.0). cc @simonjbeaumont

[simonjbeaumont]

@ktoso wrote:

Okey so the intent of "extras" originally was "things that are not core to tracing and also are not stable" etc.

Today the world has moved on a bit and semantic conventions are stable and versioned: https://opentelemetry.io/docs/specs/semconv/

I would suggest we rename this -extras package into "swift-distributed-tracing-otel-semantic-conventions" and have a target per convention group (like HTTP).
...

Maybe we can include not stable ones as well and just _ prefix them etc.

Yeah, I'm +1 for putting this all this in one place and either _ or name them experimental, evolving with OTel spec, as it evolves.

@czechboy0 wrote:

I'm also curious what @slashmo would think about this becoming non-tracing specific, to be able to include e.g. metrics OTel conventions (we could rename accordingly, since this is pre-1.0). cc @simonjbeaumont

Again, yes. OTel is more than traces, so we should centralise all the semantic conventions in one place IMO.

However, IMO that place should be the swift-otel package and we stop having an extra package.

My guess is this package is here because we didn't want folks to have to depend on a backend package when they were only instrumenting their code and that this package is intended to be used in conjunction with the API package(s) only.

However, I think this is a discoverability issue and I would propose we just make this a separate target in swift-otel.

I often try to think about the adopter trying to glue all this together and how they will even discover where the functionality lives.

1. Wanna instrument your library? Makes sense to reach for the swift-log, swift-metrics, swift-distributed-tracing packages.
2. Want to add some OTel-isms? Makes sense to reach for the swift-otel package.
3. Want to bootstrap your observability subsystems to export to an OTel Collector? Makes sense to reach for the swift-otel package.

It's just for (2) and (3) you'll use different parts of what the swift-otel package offers. And that's fine, we can let target-based resolution take care of not bringing in the backend dependencies for folks that only want to instrument with the OTel conventions.

To be fair, swift-otel already has precedent for this because it provides some useful functions on the logger, which add span attributes to logs regardless of what backend you're using—this functionality predates the logging support in swift-otel.

[NeedleInAJayStack]

I would suggest we rename this -extras package into "swift-distributed-tracing-otel-semantic-conventions" and have a target per convention group (like HTTP).

I love that idea!

I'm also curious what @slashmo would think about this becoming non-tracing specific, to be able to include e.g. metrics OTel conventions (we could rename accordingly, since this is pre-1.0).

Yeah, I totally agree here too.

My guess is this package is here because we didn't want folks to have to depend on a backend package when they were only instrumenting their code and that this package is intended to be used in conjunction with the API package(s) only.

However, I think this is a discoverability issue and I would propose we just make this a separate target in swift-otel.

I would actually softly and respectfully disagree. The fact that swift-distributed-tracing does not include a backend (and all related dependencies, including NIO and grpc) is actually a big reason why I'm using it in libraries instead of opentelemetry-swift. While I wish target-based resolution worked cleanly, today SPM resolves all dependencies of a dependency package (not target) during development, so packages with more dependencies introduce more possibilities of conflicts. It seems like discoverability could be solved in other ways, including links in READMEs or the SSWG package list. But I will defer to you guys since I'm kinda new around here.

Obviously some of the changes discussed are beyond what I have permissions to do, so let me know how I can be helpful! Thanks for the thoughtful conversation!

[simonjbeaumont]

I would actually softly and respectfully disagree.

That's great—I think it's important we discuss the tradeoffs before making a decision here :)

While I wish target-based resolution worked cleanly, today SPM resolves all dependencies of a dependency package (not target) during development.

While I believe there may have been historical issues with this, it's my understanding that this has reliably worked for some time and we are relying on this in some other packages.

I have some experiments that show this to be working. Specifically, I'm claiming that, given the following scenario...

• Package swift-a provides library product A.
• Package swift-b provides library product BSmall, which does not depend on A.
• Package swift-b provides library product BBig, which does depend on A.
• Package swift-c depends on BSmall.

...performing a build in swift-c does not build A, nor even clone swift-a.

Do you have a scenario where this is not true? If so, could you provide some details?

My proposal is that libraries that wish to add tracing would depend on Tracing from swift-distributed-tracing only. And for libraries that wish to add OTel conventional spans would depend on OTelSemanticConventions from swift-otel (this library product does not currently exist). That library would not result in resolving NIO or gRPC, because this proposed new library product would not depend on them.

The fact that swift-distributed-tracing does not include a backend (and all related dependencies, including NIO and grpc) is actually a big reason why I'm using it in libraries instead of opentelemetry-swift.

We should be careful not to conflate swift-otel and opentelemetry-swift in this discussion. It's possibly that opentelemetry-swift is not structured in the way I describe above which would not make affordances for only resolving what you need for semantic conventions (although I haven't checked).

[NeedleInAJayStack]

Specifically, I'm claiming that, given the following scenario...

• Package swift-a provides library product A.
• Package swift-b provides library product BSmall, which does not depend on A.
• Package swift-b provides library product BBig, which does depend on A.
• Package swift-c depends on BSmall.

...performing a build in swift-c does not build A, nor even clone swift-a.

Ah, thank you for this excellent outline! Yes, this is exactly the situation I was referring to but explained much more clearly!

Do you have a scenario where this is not true? If so, could you provide some details?

Yes, I believe that swift-a is cloned, but not built in this situation. I've recreated the scenario above here with swift-c renamed to SpmCacheRecreatorC, and the same for swift-a and swift-b. When building C, I see that swift-a is cloned, resolved, and shown as a dependency. Here's the view in XCode (note SpmCacheRecreatorA in the dependencies):

And from the command line:

Here's a screenshot showing that A is not built as part of the C build process:

To outline my concern in the context of this example: Having BBig and BSmall in the same package means that swift-c inherits swift-b's version compatibility with swift-a, even though it is not used. This increases the possibility of version incompatibilities for any package that might import C and A separately. Another minor concern is that this increases C's initial build time due to cloning an unused swift-a.

[simonjbeaumont]

Do you have a scenario where this is not true? If so, could you provide some details?

Yes, I've recreated the scenario above here with swift-c renamed to SpmCacheRecreatorC, and the same for swift-a and swift-b. Note that when building C, swift-a is certainly cloned, resolved, and shown as a dependency. Here's the view in XCode:

Thank you for taking the time to put this together. I'll take a closer look at this later on. If we can't get the target-based resolution to reliably work, then I agree we'll need to much more cautious in our approach to the package structure.

[simonjbeaumont]

@NeedleInAJayStack I have taken a look at your repro of the target-based resolution flow and better understand it.

Previously I had claimed this:

...given the following scenario...

1. Package swift-a provides library product A.
2. Package swift-b provides library product BSmall, which does not depend on A.
3. Package swift-b provides library product BBig, which does depend on A.
4. Package swift-c depends on BSmall.

...performing a build in swift-c does not build A, nor even clone swift-a.

As you have pointed out, only one of the final claims is true: A is not built, but swift-a is cloned and resolved.

This was because BBig was a library product and so, once swift-c has a package dependency on swift-b it may well add a target dependency on BBig and so it got resolved.

I had misremembered how we relied on this for other packages, where A would be depended on in swift-b only in either internal library targets or test targets. I confirmed that with your repo packages: if you remove the product definition of BBig but keep the target BBig and retag, then resolving the swift-c no longer needs to clone swift-a.

I fell into the trap of thinking it went one-level deeper and only resolved based on which products were used in the graph. Probably because of the wording of the target-based resolution proposal:

We propose to enhance the dependency resolution process to resolve only the dependencies that are actually being used in the package graph. The resolution process can examine the target dependencies to figure out which package dependencies require resolution. Since we will only resolve what is required, it may reduce the odds of dependency hell situations.¹

☝️ This sounds exactly what we want but I guess it doesn't do what I always expect it to do 😅.

To outline my concern in the context of this example: Having BBig and BSmall in the same package means that swift-c inherits swift-b's version compatibility with swift-a, even though it is not used. This increases the possibility of version incompatibilities for any package that might import C...

That's fair. If these packages use incompatible version constraints, there would be a problem. If they are all using from: or upToNextMajor: then it's less likely, but I concede the point.

...and A separately. Another minor concern is that this increases C's initial build time due to cloning an unused swift-a.

That is also a fair concern.

Footnotes

1. https://github.com/swiftlang/swift-evolution/blob/main/proposals/0226-package-manager-target-based-dep-resolution.md#package-manager-target-based-dependency-resolution ↩

[simonjbeaumont]

We propose to enhance the dependency resolution process to resolve only the dependencies that are actually being used in the package graph. The resolution process can examine the target dependencies to figure out which package dependencies require resolution. Since we will only resolve what is required, it may reduce the odds of dependency hell situations.1

☝️ This sounds exactly what we want but I guess it doesn't do what I always expect it to do 😅.

OK, this explains it; it's target-based dependency resolution was only ever partially implemented:

Status: Partially implemented (Swift 5.2): Implemented the manifest API to disregard targets not concerned by any dependency products, which avoids building dependency test targets.

[NeedleInAJayStack]

That's awesome background on the evolution. I appreciate you exploring this situation with me! 🙂

[NeedleInAJayStack]

Hey @ktoso @czechboy0 @simonjbeaumont I got some time this weekend and pulled together a repo that automatically generates Swift code for OTEL semantic conventions: https://github.com/NeedleInAJayStack/swift-otel-semconv

I'd be happy to contribute it as a jumping off point for the effort we discussed above. Just let me know if you're interested!

[czechboy0]

That's awesome, @NeedleInAJayStack - I think that's the right solution for the base layer. We might potentially filter just a subset of the areas, and there's a question about keeping the list up-to-date, otherwise I'm in support of exploring this approach further.