opentelemetry-http: Set context correctly for the server request body #3212

bryce-anderson · 2025-03-26T18:14:09Z

Motivation:

We need to also consider the request body when tracking signals and setting context.

Modifications:

Properly track the response and account for the NettyHttpServer auto-drain behavior as well.

...etalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ScopeTrackerV2.java

...etry-http/src/main/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilter.java

…pLifecycleObserver

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/NettyHttpServer.java

…pLifecycleObserver

bryce-anderson · 2025-04-04T23:04:24Z

Back to draft: I'm still seeing flakyness on the error pathways associated with the span completion happening before the onExchangeFinally operations. ☹️

idelpivnitskiy · 2025-04-04T23:06:47Z

...-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilterTest.java

+            request.trailers().set("x-request-trailer", "request-trailer");
+            request.payloadBody().writeAscii("bar");
+            client.request(request).toFuture().get();
+            Thread.sleep(SLEEP_DURATION);


What are those other events that may happen on server after response is received by the client? Is there any chance we can latch on some callback (onExchangeFinally or maybe on connection close) instead of using a sleep?

Can we either wait for a server to close (if that helps with races) or add a filter as the first one in the pipeline to track when it's time to start asserting attributes?

Unfortunately, we need to give the traces time to materialize and I'm not sure a filter is going to help. In the agent there are methods to the tun of waitAndAssertTraces (here) which will give the traces some grace period to materialize but for whatever reason that doesn't seem to exist for the SDK testing tools.

I mostly tried to understand what's delaying those events materialization. If it's because server-side request events may still happen after ServerContext is closed, it seems concerning and maybe we need to investigate separately why it happens. But if it's some buffering inside OTEL SDK or testing tools, then it's ok.

...-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilterTest.java

...emetry-http/src/main/java/io/servicetalk/opentelemetry/http/AbstractOpenTelemetryFilter.java

...etry-http/src/main/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilter.java

...emetry-http/src/main/java/io/servicetalk/opentelemetry/http/AbstractOpenTelemetryFilter.java

bryce-anderson · 2025-04-16T18:38:43Z

The build of 0d583df (disable offloading of the service call) demonstrates that if we remove offloading we have a stable build. However, this is not a realistic behavior for the majority of services, but refactoring the offloading behavior is something we should defer to another PR.

idelpivnitskiy

LGTM, only minor comments and questions about Thread.sleep:

...icetalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ScopeTracker.java

idelpivnitskiy · 2025-04-18T18:30:43Z

...icetalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ScopeTracker.java

 import static java.util.Objects.requireNonNull;

-class ScopeTracker implements TerminalSignalConsumer {
+final class ScopeTracker implements TerminalSignalConsumer {


Definitely not in this PR, but maybe later we can offer tracker as a public utility for other use-cases

idelpivnitskiy · 2025-04-18T18:42:47Z

...-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilterTest.java

+            response.cancel(true);
+        });
+        // For the HTTP/1.x server, we don't necessarily see the cancellation until we shutdown the server.
+        Thread.sleep(SLEEP_DURATION);


Is sleep still required after ServerContext is closed?

idelpivnitskiy · 2025-04-18T18:44:56Z

...-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilterTest.java

+            request.trailers().set("x-request-trailer", "request-trailer");
+            request.payloadBody().writeAscii("bar");
+            client.request(request).toFuture().get();
+            Thread.sleep(SLEEP_DURATION);


Can we either wait for a server to close (if that helps with races) or add a filter as the first one in the pipeline to track when it's time to start asserting attributes?

...-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilterTest.java

...etry-http/src/main/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilter.java

bryce-anderson · 2025-04-18T22:50:07Z

@idelpivnitskiy, wrt sleep(), how much is that worth to you? It's not a long sleep, I think we can afford it, and investigating alternative ways to avoid it seems to me like more effort than they're worth.

Add a server test

74b4ea4

bryce-anderson requested a review from idelpivnitskiy March 26, 2025 18:14

idelpivnitskiy reviewed Mar 26, 2025

View reviewed changes

...etalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ScopeTrackerV2.java Outdated Show resolved Hide resolved

Now its working

05114f3

bryce-anderson force-pushed the bl_anderson/ServerHttpLifecycleObserver branch from 74e7a87 to 05114f3 Compare March 26, 2025 20:07

bryce-anderson changed the title ~~Bl anderson/server http lifecycle observer~~ WIP: Set context correctly for the server request body Mar 26, 2025

bryce-anderson commented Mar 26, 2025

View reviewed changes

...etalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ScopeTrackerV2.java Outdated Show resolved Hide resolved

bryce-anderson commented Mar 26, 2025

View reviewed changes

...etry-http/src/main/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServerFilter.java Outdated Show resolved Hide resolved

bryce-anderson added 13 commits March 28, 2025 14:54

Merge remote-tracking branch 'origin/main' into bl_anderson/ServerHtt…

d1a9d3a

…pLifecycleObserver

Some refinements, but still need some more tests

691a4ce

Adding more tests

c250a98

We need to adjust the NettyHttpServer

bdaced0

Another test

bab1e55

Merge remote-tracking branch 'origin/main' into bl_anderson/ServerHtt…

b312ddb

…pLifecycleObserver

Also ensure context is correct if the service returns an error

a598ba7

More progress

39214f8

Tests work

5d3fa64

Cleanup

8c4cf85

Some more cleanup

d88ff28

Some cleanup and docs for flaky tests

9eff5d4

Add back comment

aaf5603

bryce-anderson commented Apr 1, 2025

View reviewed changes

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/NettyHttpServer.java Outdated Show resolved Hide resolved

bryce-anderson marked this pull request as ready for review April 1, 2025 15:35

Fix buildHealth

3d00370

bryce-anderson changed the title ~~WIP: Set context correctly for the server request body~~ opentelemetry-http: Set context correctly for the server request body Apr 1, 2025

bryce-anderson marked this pull request as draft April 1, 2025 17:20

bryce-anderson added 4 commits April 4, 2025 15:50

Merge remote-tracking branch 'origin/main' into bl_anderson/ServerHtt…

7a31225

…pLifecycleObserver

Fix test

e2940ec

Cleanup and fix a flaky test

29bf7c9

stylecheck

327b338

bryce-anderson requested a review from idelpivnitskiy April 4, 2025 22:10

bryce-anderson marked this pull request as draft April 4, 2025 23:03

idelpivnitskiy reviewed Apr 5, 2025

View reviewed changes

bryce-anderson added 7 commits April 7, 2025 10:17

See if this makes the tests stable

42ff93f

Cleanup

c229e57

Some feedback

0540145

This is similar to the other context testing

c019f29

Merge branch 'main' into bl_anderson/ServerHttpLifecycleObserver

9c3a1b7

Some cleanup

0c5f530

Cleanup

9e524b8

bryce-anderson force-pushed the bl_anderson/ServerHttpLifecycleObserver branch 2 times, most recently from 39d73d9 to 9d381a3 Compare April 15, 2025 23:37

Test something

01f2ada

bryce-anderson force-pushed the bl_anderson/ServerHttpLifecycleObserver branch from 9d381a3 to 01f2ada Compare April 16, 2025 16:23

bryce-anderson added 2 commits April 16, 2025 10:59

More repeats to see if its really stable

a7a673d

Reduce the number of repeats so we actually finish sometime this year

0d583df

bryce-anderson added 3 commits April 16, 2025 15:59

Cleanup

b451bed

Cleanup

89b9578

Only check attributes if we are not offloading

9881c8a

bryce-anderson requested a review from idelpivnitskiy April 16, 2025 23:49

bryce-anderson marked this pull request as ready for review April 16, 2025 23:49

idelpivnitskiy reviewed Apr 18, 2025

View reviewed changes

Some cleanups

256e3bb

bryce-anderson requested a review from idelpivnitskiy April 18, 2025 22:48

idelpivnitskiy approved these changes Apr 19, 2025

View reviewed changes

bryce-anderson merged commit f1e4b36 into apple:main Apr 21, 2025
12 checks passed

bryce-anderson deleted the bl_anderson/ServerHttpLifecycleObserver branch April 21, 2025 15:38

opentelemetry-http: Set context correctly for the server request body #3212

opentelemetry-http: Set context correctly for the server request body #3212

Uh oh!

Conversation

bryce-anderson commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryce-anderson commented Apr 4, 2025

Uh oh!

idelpivnitskiy Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

bryce-anderson Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryce-anderson commented Apr 16, 2025

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

idelpivnitskiy Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bryce-anderson commented Apr 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bryce-anderson commented Mar 26, 2025 •

edited

Loading

bryce-anderson Apr 18, 2025 •

edited

Loading