refactor(instrumentation-http): Add back support for http semconv#5665
Conversation
At this point it is mostly copy-paste from when most of it was removed, but there have been a few features since then (synthetic) and also a few things were dropped and re-added already like metrics. This also uses the new utility for semconv stability instead of the old hardcoded enum. Some things are still missing according to tests so will need another pass. Doing this all in one PR is... a lot. Possibly too much, but not sure if it is easier to split either. It is just a lot of change.
| | [`requireParentforOutgoingSpans`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L103) | Boolean | Require that is a parent span to create new span for outgoing requests. | | ||
| | [`requireParentforIncomingSpans`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L105) | Boolean | Require that is a parent span to create new span for incoming requests. | | ||
| | [`headersToSpanAttributes`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L107) | `object` | List of case insensitive HTTP headers to convert to span attributes. Client (outgoing requests, incoming responses) and server (incoming requests, outgoing responses) headers will be converted to span attributes in the form of `http.{request\|response}.header.header_name`, e.g. `http.response.header.content_length` | | ||
| Http instrumentation has a few [configuration options](https://github.com/open-telemetry/opentelemetry-js/blob/e1ec4026edae53a2dea3a9a604d6d21bb5e8d99f/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L60-L93) available to choose from. |
There was a problem hiding this comment.
Note for reviewer: I removed the links from the table and put a general link up here. It makes the table easier to work with, but more importantly, the links were no longer linking to the right place and they are fragile anyway.
There was a problem hiding this comment.
+1 from me. I did the same in other instrumentation READMEs. Those links are a maintenance pain.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5665 +/- ##
=======================================
Coverage 95.03% 95.03%
=======================================
Files 310 310
Lines 7988 7992 +4
Branches 1607 1614 +7
=======================================
+ Hits 7591 7595 +4
Misses 397 397 🚀 New features to boost your workflow:
|
|
I'm not quite sure how to handle the changelog. My first thought is to Skip Changelog, because it should just be reimplementing behavior that was already in the previous release. But there is the other PR in Breaking Changes, so we have to somehow offset the two. |
trentm
left a comment
trentm
left a comment
There was a problem hiding this comment.
Nice! A few nits is all. I think this is good.
| [ATTR_NET_PEER_NAME]: hostname, | ||
| [ATTR_HTTP_HOST]: headers.host ?? `${hostname}:${port}`, | ||
| }; | ||
|
|
There was a problem hiding this comment.
nit unrelated to this change: I notice that #5488 added ATTR_USER_AGENT_ORIGINAL to newAttributes.
I think it probably should not have. Notice that newAttributes has a comment "Opt-in attributes left off for now" and per https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client-span user_agent.original is Opt-in.
Possibly if enableSyntheticSourceDetection is configured, this could be considered as an Opt-in to also capture the raw user_agent.original.
Anyway, not for this PR.
The end result for release will be that there was churn in instr-http that should just amount to a refactor. So I'd move the earlier breaking-change one to a |
JamieDanielson
changed the title
Co-authored-by: Trent Mick <trentm@gmail.com>
|
Thanks for the review and notes @trentm ! I've made the changes suggested (holding off on the user agent change since it went in separately as mentioned). Let me know if you see anything else. |
trentm
left a comment
trentm
left a comment
There was a problem hiding this comment.
LGTM with a suggestion/correction for the content-length-related attrs.
…E.md Co-authored-by: Trent Mick <trentm@gmail.com>
…en-telemetry#5665) Co-authored-by: Trent Mick <trentm@gmail.com>
3 participants
Which problem is this PR solving?
Short description of the changes
See #5646. This is re-implementing the support for dual-emit HTTP semantic conventions. I couldn't do a revert, because other changes have gone in since legacy attributes were removed. Much of this is copy-pasted though from the previous implementation, with some additional changes related to the new synthetic attributes, stable outgoing metrics, and shared utility for SemconvStability.
Type of change
How Has This Been Tested?
Unit tests
Checklist: