feat: Harmonize HTTP client instrumentation

[tt]

This attempts to harmonize the HTTP client instrumentation by adopting the latest semantic conventions, version 1.26.0.

Each gem currently sets a slightly different set of attributes which makes it harder to create good dashboards in an environment where more than one HTTP client is used.

My change adopts the latest semantic conventions but doesn't retire what's already there. That seems like a better fit for the next major release which will allow a seamless migration.

Notably, this pull request makes the following HTTP client span attributes available:

• http.request.method
• http.response.status_code
• server.address
• server.port
• url.full
• url.scheme

There are no constants available for these yet as the opentelemetry-semantic_conventions gem was last updated in open-telemetry/opentelemetry-ruby@159044d to version 1.10.0.

See also this summary of changes and the list of deprecated attributes.

[linux-foundation-easycla]

• ✅login: tt / (39221d1)
• ✅login: tt / (39221d1, 2ac3f83)
• ✅login: tt / (39221d1, 2ac3f83, a42aa74)
• ✅login: tt / (39221d1, 2ac3f83, a42aa74, 49d0d63)
• ✅login: tt / (39221d1, 2ac3f83, a42aa74, 49d0d63, ea385d7)
• ✅login: tt / (39221d1, 2ac3f83, a42aa74, 49d0d63, ea385d7, 6a6d46d)

The committers listed above are authorized under a signed CLA.

[tt]

I saw to use conventional commit messages but also that this will be squash merged. I used the style for the pull request title and not for individual commits but happy to change it if necessary.

[arielvalentin]

Thank you for your contribution but unfortunately, we cannot accept this PR in its current state.

We have not adopted 1.x semconv in our instrumentations because we want to be able to be able to generally support schema URLs¹ as well as the OTEL_SEMCONV_STABILITY_OPT_IN environment variables as described in the specification:

Warning Existing HTTP instrumentations that are using v1.20.0 of this document (or prior):

SHOULD NOT change the version of the HTTP or networking conventions that they emit until the HTTP semantic conventions are marked stable (HTTP stabilization will include stabilization of a core set of networking conventions which are also used in HTTP instrumentations). Conventions include, but are not limited to, attributes, metric and span names, and unit of measure.
SHOULD introduce an environment variable OTEL_SEMCONV_STABILITY_OPT_IN in the existing major version which is a comma-separated list of values. The only values defined so far are:
http - emit the new, stable HTTP and networking conventions, and stop emitting the old experimental HTTP and networking conventions that the instrumentation emitted previously.
http/dup - emit both the old and the stable HTTP and networking conventions, allowing for a seamless transition.
The default behavior (in the absence of one of these values) is to continue emitting whatever version of the old experimental HTTP and networking conventions the instrumentation was emitting previously.
Note: http/dup has higher precedence than http in case both values are present
SHOULD maintain (security patching at a minimum) the existing major version for at least six months after it starts emitting both sets of conventions.
SHOULD drop the environment variable in the next major version.²

Though I recognize this is extremely challenging, many of our users are relying on pre 1.0 schema name, so having the ability to support http/dup would be important for us to provide during a transition.

In addition to that, many of the HTTP conventions are still considered mixed stability. We've been waiting a bit for the specification to settle before committing to anything and have had a few experiments to support multiple versions of the schema but have not had much success.

I think what that means for this PR is that it would have to change to support the envars before we are willing to provide initial feedback and consider including it in a release.

If you have an immediate need to support semconv 1.20+ then I would recommend looking at performing schema migrations in the otel collector either using a Schema Processor or a Transform Processor:

• https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/schemaprocessor
• https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/transformprocessor

Footnotes

1. https://opentelemetry.io/docs/specs/otel/schemas/ ↩

2. https://opentelemetry.io/docs/specs/semconv/http/ ↩

[tt]

@arielvalentin:

Though I recognize this is extremely challenging, many of our users are relying on pre 1.0 schema name, so having the ability to support http/dup would be important for us to provide during a transition.

I agree this is important. I also want overlap for our current traces. That said, isn't this essentially the same as what I've done where both sets of attributes are being included?

I think the downside of not supporting these new conventions is that people starting on their observability journey today will have to go through a migration that could be avoided.

Anyway, it seems you could achieve this entirely through releases:

1. Version 1.0 contains old set ("default behavior")
2. Version 1.1 contains old and new set (http/dup equivalent)
3. Version 2.0 contains new set (http equivalent)

I can certainly add OTEL_SEMCONV_STABILITY_OPT_IN support if that's the preferred direction but I'm probably not going to do it for all HTTP clients. We fortunately don't use all of them. 😅

[tt]

@arielvalentin, any appetite for converging the existing instrumentation towards the now deprecated attributes?

For instance, look at the instrumentation for Excon and Faraday:

opentelemetry-ruby-contrib/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb

Lines 29 to 36 in aedc42c

	attributes = {
	OpenTelemetry::SemanticConventions::Trace::HTTP_METHOD => http_method,
	OpenTelemetry::SemanticConventions::Trace::HTTP_SCHEME => datum[:scheme],
	OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET => datum[:path],
	OpenTelemetry::SemanticConventions::Trace::HTTP_HOST => datum[:host],
	OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME => datum[:hostname],
	OpenTelemetry::SemanticConventions::Trace::NET_PEER_PORT => datum[:port]
	}

opentelemetry-ruby-contrib/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb

Lines 49 to 55 in da831f0

	instrumentation_attrs = {
	'http.method' => http_method,
	'http.url' => OpenTelemetry::Common::Utilities.cleanse_url(url.to_s),
	'faraday.adapter.name' => app.class.name
	}
	instrumentation_attrs['net.peer.name'] = url.host if url.host
	instrumentation_attrs['peer.service'] = config[:peer_service] if config[:peer_service]

These are mostly different sets. I don't strictly need the newest conventions but having a common set of attributes would be really valuable.

[arielvalentin]

any appetite for converging the existing instrumentation towards the now deprecated attributes?

Yes. I love symmetry and very much dislike surprises.

These instrumentations where likely written by different authors and we don't exactly have a "checklist" of what an instrumentation should emit but only that it should do the absolute minimum or order to add instrumentation.

In some of those cases there is a bit of divergence in the data provided by the libraries themselves.

In the example you referenced above, Faraday and Excon, one provides the full URL (http.url) while the other only provides the path (http.target)¹.

I assert the authors were likely trying to avoid any additional object allocation and only provided the minimal data made available to the instrumentation by the library itself; so, it's a tradeoff here but depending on system load it may not be so bad.

Faraday is another one of the odd cases, where those attributes really matter most to the underlying adapter client spans. Unlike the underlying adapters, Faraday provides a much nicer and symmetric abstraction since it has a URL object, where you may be able to extract all of the attributes and share them with the adapter spans by amending them to the OpenTelemetry::Common::HTTP::ClientContext shared attributes helper:

opentelemetry-ruby-contrib/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb

Line 58 in 2230403

OpenTelemetry::Common::HTTP::ClientContext.attributes

What should we do next?

I am in favor of providing symmetric attributes in HTTP instrumentations, but if we could do so by avoiding allocating any additional objects in drivers that would be best.

If Faraday were to include all of the attributes as part of OpenTelemetry::Common::HTTP::ClientContext, would that address your use case?

If it does, then how would you feel about approaching it that way instead of updating individual drivers?

Footnotes

1. https://opentelemetry.io/docs/specs/semconv/attributes-registry/http/#http-deprecated-attributes ↩

[tt]

@arielvalentin:

If Faraday were to include all of the attributes as part of OpenTelemetry::Common::HTTP::ClientContext, would that address your use case?

I need a bit of help understanding this. Isn't OpenTelemetry::Common::HTTP::ClientContext for passing additional context in?

I think I understand it now: You would make it the responsibility of the adapter to then add the attributes to the span. I guess that might work based on which adapter is used.

My most immediate problem was probably the lack of http.url in the Excon instrumentation so I opened a pull request for that change.

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot