Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context propagation requirements to HTTP conventions #1783

Closed
wants to merge 6 commits into from
Closed

Add context propagation requirements to HTTP conventions #1783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6487af5
Add context propagation requriements to HTTP conventions
lmolkova Jun 29, 2021
d40ab59
changelog
lmolkova Jun 29, 2021
71d620c
add examples
lmolkova Jul 1, 2021
ee9d872
up
lmolkova Jul 1, 2021
bdc4cda
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
lmolkova Jul 9, 2021
ff86380
instrumentation that injects http header must also emit http span
lmolkova Jul 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ release.
([#1781](https://github.com/open-telemetry/opentelemetry-specification/pull/1781))
- Remove `rpc.jsonrpc.method`, clarify that `rpc.method` should be used instead.
([#1748](https://github.com/open-telemetry/opentelemetry-specification/pull/1748))
- Add context propagation requirements to HTTP spec.
([#1783](https://github.com/open-telemetry/opentelemetry-specification/pull/1783))

### Compatibility

Expand Down
28 changes: 28 additions & 0 deletions specification/trace/semantic_conventions/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ and various HTTP versions like 1.1, 2 and SPDY.
- [Status](#status)
- [Common Attributes](#common-attributes)
- [HTTP client](#http-client)
- [Context propagation](#context-propagation)
- [Example 1: context propagation](#example-1-context-propagation)
- [Example 2: back-off](#example-2-back-off)
- [HTTP server](#http-server)
- [HTTP server definitions](#http-server-definitions)
- [HTTP Server semantic conventions](#http-server-semantic-conventions)
Expand Down Expand Up @@ -111,6 +114,31 @@ from the `net.peer.name`
used to look up the `net.peer.ip` that is actually connected to.
In that case it is strongly recommended to set the `net.peer.name` attribute in addition to `http.host`.

### Context propagation

`Context` created for HTTP client span (if valid) MUST be injected on outgoing request headers using configured [propagator](../../context/api-propagators.md). Instrumentation that injects `Context` into HTTP request headers MUST also emit a span that complies with other client requirements in this specification.

**Exception**: if outgoing HTTP request already has valid `Context` (for configured propagator), it cannot be changed and new span MUST NOT be recorded.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this be determined using the current OpenTelemetry API? Would all http client instrumentation be required to query the propagator, grab the fields() and check to see if they exist already? How would that work if the http request object was being re-used and the fields were left over from a previous request? (see the documentation on the fields() function of the propagator to see what I'm referring to).

Copy link
Contributor Author

@lmolkova lmolkova Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this be determined using the current OpenTelemetry API?

I was thinking about having this logic within the propagator and have 2 options:

  1. Sub-optimal, no propagator change requires: instrumentation calls extract and checks if it got valid context. It's cheap when there is one layer of instrumentation, but if there are multiple, it involves extra context parsing and allocation.
  2. Optimal: add CanExtract (naming is always hard) method that only checks for context presence and may do minimum validity checks.

This way decision on context presence and validity stays within the propagator and multiple HTTP (and other protocol) instrumentations don't have to do it.

I suggest starting with 1 as perf hit affects relatively rare scenarios (multiple instrumentation layers) and is not very big in the general case of single transparent. Optimization (option 2) can come later.
I'm also open to starting with option 2 right away add a new method on propagators if there is any consensus around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would that work if the http request object was being re-used and the fields were left over from a previous request?

Great point! I believe the current spec requires to clean up the context for the reused carriers, Is there an assumption that it's not always possible?

I think this is also worth mentioning here that context should be cleaned up after HTTP try if the HTTP client allows reusing the same request instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says "successive calls should clear these fields first.", so your proposed call to extract would still be operating on the previous contents of the headers, meaning that the resulting Context would always be valid, so I don't think this is a feasible approach, with either options 1 or 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you're right. what's the feeling about changing it to clean up after rather than before? this way everyone cleans up for what they've done and never breaks anything else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh. I have no idea...I don't think I've ever seen a web client that re-uses request objects like this, so I don't have the context for what it's even referring to. :)

Copy link
Contributor Author

@lmolkova lmolkova Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems it was there from the very beginning (#147) and there was no discussion/explanation around this choice. It also seems that at least golang http client allows to reuse http request instances between tries. I'll raise it as a separate issue and until then this PR is blocked.


#### Example 1: context propagation

There is no valid W3C Trace Context on the request when instrumentation starts, W3C Trace Context propagator is configured on the instrumentation.

1. Instrumentation checks if a valid context can be extracted using W3C Trace-Context propagator
- no, there is no W3C Trace Context
- OR: yes, there is an invalid W3C Trace Context
2. Instrumentation starts a span
3. Instrumentation injects context of the new span using the W3C Trace Context propagator
4. Instrumentation waits for call to complete and ends span

#### Example 2: back-off

There is valid W3C Trace Context context on the request when instrumentation starts. W3C Trace Context propagator is configured on the instrumentation.

1. Instrumentation checks if valid context can be extracted using W3C Trace Context propagator
- yes, there is a valid W3C Trace Context
2. Instrumentation lets call to complete without creating a span

## HTTP server

To understand the attributes defined in this section, it is helpful to read the "Definitions" subsection.
Expand Down