tracing: add new ability to customize span operation with formatter by wbpcode · Pull Request #42303 · envoyproxy/envoy

wbpcode · 2025-11-29T06:52:32Z

Commit Message: tracing: add new ability to customize span operation with formatter
Additional Description:

To close #42288

Risk Level: low.
Testing: unit.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <wbphub@gmail.com>

repokitteh-read-only · 2025-11-29T06:52:43Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #42303 was opened by wbpcode.

see: more, trace.

wbpcode · 2025-11-29T06:53:39Z

cc @zirain

Signed-off-by: wbpcode <wbphub@gmail.com>

wbpcode · 2025-11-30T01:40:28Z

/retest

… dev-span-operation

wbpcode · 2025-12-01T01:45:37Z

/retest

ravenblackx · 2025-12-04T15:26:44Z

    }

-    EXPECT_CALL(config, modifySpan(_)).WillOnce(Invoke([this](Span& span) {
+    EXPECT_CALL(config, modifySpan(_, _)).WillOnce(Invoke([this](Span& span, bool) {


FYI (no need for a change here but you can if you want), when doing an EXPECT_CALL with all the args wildcarded, you can just do

EXPECT_CALL(config, modifySpan)

instead of

EXPECT_CALL(config, modifySpan(_, _))

which can sometimes make this sort of update require fewer touches. In this case it wouldn't help much because the action invokes a function anyway so you'd have to change the args there, but still, slightly less fragile.

I never learned this before. Good suggestion.

ravenblackx · 2025-12-04T15:31:05Z

+    if (operation_ != nullptr) {
+      return makeOptRef<const Formatter::Formatter>(*operation_);
+    }
+    return OptRef<const Formatter::Formatter>();


I think this block (and same for upstreamOperation below) could just be return makeOptRefFromPtr(operation_.get());

ravenblackx · 2025-12-04T15:50:56Z

    - source/common/event/file_event_impl.cc
    - source/common/http/async_client_impl.cc
    - source/common/grpc/google_async_client_impl.cc
+    - source/common/tracing/tracer_config_impl.h


Could you move the implementation of the ConnectionManagerTracingConfig constructor into a cc file so that this exception is legitimate here? (Also because it's grown to be a pretty big constructor to be implemented in a header file.)
Otherwise the exception should be in the block above labeled

# Header files that can throw exceptions. These should be limited; the only # valid situation identified so far is template functions used for config # processing.

(Even better would be to do the StatusOr thing and the validation in the function that calls the constructor and transform it to an exception a little bit further up the callstack where things are already thrown, so that a new exception isn't needed.)

… dev-span-operation

Signed-off-by: wbpcode <wbphub@gmail.com>

… dev-span-operation

Signed-off-by: wbpcode <wbphub@gmail.com>

… dev-span-operation

Signed-off-by: wbpcode <wbphub@gmail.com>

Signed-off-by: code <wbphub@gmail.com>

zirain · 2025-12-10T00:18:16Z

/retest

… dev-span-operation

…/envoy into dev-span-operation

wbpcode · 2025-12-10T06:25:34Z

/retest

wbpcode · 2025-12-10T13:08:51Z

gently ping @adisuissa for an API review.

adisuissa

Thanks!
Overall API lgtm. Left some minor comments

adisuissa · 2025-12-10T15:20:22Z

    enum OperationName {
      // The HTTP listener is used for ingress/incoming requests.
-      INGRESS = 0;
+      INGRESS = 0 [deprecated = true, (envoy.annotations.deprecated_at_minor_version_enum) = "3.0"];


These changes are not needed. If there was a field using the enum, it should've been deprecated. The not-implemented-hide is sufficient.

Coming to think about it again, if this enum isn't used anywhere, what will be the implications of removing that completely?

Coming to think about it again, if this enum isn't used anywhere, what will be the implications of removing that completely?

We rarely remove API and only mark it as un-implemented and deprecated to ensure it will not be used again. But, yeah, maybe not-implemented-hide is sufficient.

I was also going to suggest removing it completely (since the field that used it has been removed completely), but the enum is still used in code. I think probably those usages should be cleaned up too, but that seems like a followup thing rather than something to include in this PR.

adisuissa · 2025-12-10T16:32:13Z

+  //
+  // The same :ref:`format specifier <config_access_log_format>` as used for
+  // :ref:`HTTP access logging <config_access_log>` applies here, however
+  // unknown specifier values are replaced with the empty string instead of ``-``.


OOC why is there an edge case here with replacement value for unknown specifier?
If this is for backwards compatibility, SGTM.
However, if this is something that is added because the implementation is a bit different, then I think the argument of keeping a consistent replacement strategy may outweigh the implementation complexity cost. (the main reason is that if someone has a tool that looks at traces and looks for those "-" that are now an empty string, it makes their tools harder to maintain).

It's not a new behavior. Except the access log, in all other substitution formatters (header mutation, custom tag, and so on), unknown specifier values are replaced with the empty string instead of -. Even the comment self, I copied it from header mutation/custom tag. 🤣

yeah, you have more context on formatter features than I do.

Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

…3/http_connection_manager.proto Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

Signed-off-by: wbpcode <wbphub@gmail.com>

wbpcode · 2025-12-11T06:34:26Z

/retesst

ravenblackx · 2025-12-11T16:01:13Z

/retest

adisuissa

/lgtm api

adisuissa · 2025-12-11T18:05:43Z

+  //
+  // The same :ref:`format specifier <config_access_log_format>` as used for
+  // :ref:`HTTP access logging <config_access_log>` applies here, however
+  // unknown specifier values are replaced with the empty string instead of ``-``.


yeah, you have more context on formatter features than I do.

wbpcode · 2025-12-11T23:55:43Z

/retest

zirain · 2025-12-12T00:49:29Z

/retest

… dev-span-operation

wbpcode · 2025-12-12T05:50:46Z

/retest

wbpcode · 2025-12-12T15:02:06Z

friendly ping another approval cc @ravenblackx 🌷

…nvoyproxy#42303) Commit Message: tracing: add new ability to customize span operation with formatter Additional Description: To close envoyproxy#42288 Risk Level: low. Testing: unit. Docs Changes: n/a. Release Notes: added. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: MayorFaj <mayorfaj@gmail.com>

…nvoyproxy#42303) Commit Message: tracing: add new ability to customize span operation with formatter Additional Description: To close envoyproxy#42288 Risk Level: low. Testing: unit. Docs Changes: n/a. Release Notes: added. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: Gustavo <grnmeira@gmail.com>

tracing: add new ability to customize span operation with formatter

7fcf4af

Signed-off-by: wbpcode <wbphub@gmail.com>

repokitteh-read-only Bot added the api label Nov 29, 2025

repokitteh-read-only Bot assigned markdroth Nov 29, 2025

improve coverage

f636677

Signed-off-by: wbpcode <wbphub@gmail.com>

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…

8306216

… dev-span-operation

tonya11en requested a review from ravenblackx December 2, 2025 04:57

wbpcode assigned ravenblackx Dec 3, 2025

ravenblackx requested changes Dec 4, 2025

View reviewed changes

wbpcode assigned adisuissa and unassigned markdroth Dec 5, 2025

wbpcode added 2 commits December 5, 2025 03:21

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…

0316dbe

… dev-span-operation

addressed part of comments

02a3687

Signed-off-by: wbpcode <wbphub@gmail.com>

ravenblackx added the waiting label Dec 5, 2025

ravenblackx mentioned this pull request Dec 5, 2025

[Cleanup] Move exception-throwing configs out of headers #42436

Merged

wbpcode added 2 commits December 7, 2025 09:22

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…

9a6dbfa

… dev-span-operation

address left comments

2e5ae68

Signed-off-by: wbpcode <wbphub@gmail.com>

repokitteh-read-only Bot removed the waiting label Dec 7, 2025

wbpcode added 4 commits December 7, 2025 09:42

fix build

9fb7e70

Signed-off-by: wbpcode <wbphub@gmail.com>

fix format

b55cb44

Signed-off-by: wbpcode <wbphub@gmail.com>

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…

b5a893c

… dev-span-operation

fix format

1a742db

Signed-off-by: wbpcode <wbphub@gmail.com>

ravenblackx previously approved these changes Dec 8, 2025

View reviewed changes

Merge branch 'main' into dev-span-operation

b2406e7

Signed-off-by: code <wbphub@gmail.com>

wbpcode dismissed ravenblackx’s stale review via b2406e7 December 9, 2025 08:21

ravenblackx approved these changes Dec 9, 2025

View reviewed changes

wbpcode added 2 commits December 10, 2025 01:35

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…

89a5c6a

… dev-span-operation

Merge branch 'dev-span-operation' of ssh://ssh.github.com:443/wbpcode…

298d0f7

…/envoy into dev-span-operation

ravenblackx previously approved these changes Dec 10, 2025

View reviewed changes

adisuissa reviewed Dec 10, 2025

View reviewed changes

Update api/envoy/config/route/v3/route_components.proto

ea3774c

Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

wbpcode dismissed ravenblackx’s stale review via ea3774c December 11, 2025 02:24

wbpcode and others added 2 commits December 11, 2025 10:24

Update api/envoy/extensions/filters/network/http_connection_manager/v…

796100d

…3/http_connection_manager.proto Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

address comment

38ce8b6

Signed-off-by: wbpcode <wbphub@gmail.com>

adisuissa reviewed Dec 11, 2025

View reviewed changes

repokitteh-read-only Bot removed the api label Dec 11, 2025

zirain mentioned this pull request Dec 12, 2025

allow the change altStatName for backend envoyproxy/gateway#7520

Open

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…

4cb76ca

… dev-span-operation

wbpcode enabled auto-merge (squash) December 12, 2025 09:29

ravenblackx approved these changes Dec 12, 2025

View reviewed changes

wbpcode merged commit 9747733 into envoyproxy:main Dec 12, 2025
26 checks passed

wbpcode deleted the dev-span-operation branch December 12, 2025 16:44

zirain mentioned this pull request Dec 15, 2025

REQUESTED_SERVER_NAME fallback to REQ(:AUTHORITY) #42617

Closed

NomadXD mentioned this pull request Mar 9, 2026

Expose Envoy Tracing operation and upstream_operation config options kgateway-dev/kgateway#13465

Open

Conversation

wbpcode commented Nov 29, 2025

Uh oh!

repokitteh-read-only Bot commented Nov 29, 2025

Uh oh!

wbpcode commented Nov 29, 2025

Uh oh!

wbpcode commented Nov 30, 2025

Uh oh!

wbpcode commented Dec 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zirain commented Dec 10, 2025

Uh oh!

wbpcode commented Dec 10, 2025

Uh oh!

wbpcode commented Dec 10, 2025

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wbpcode commented Dec 11, 2025

Uh oh!

ravenblackx commented Dec 11, 2025

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Dec 11, 2025

Uh oh!

zirain commented Dec 12, 2025

Uh oh!

wbpcode commented Dec 12, 2025

Uh oh!

wbpcode commented Dec 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants