xds: Add xDS node ID in few control plane errors by DNVindhya · Pull Request #11519 · grpc/grpc-java

DNVindhya · 2024-09-11T22:33:02Z

This PR adds xDS node ID in control plane errors to enable cross-referencing with control plane logs when debugging (b/364405814).

CC @fengli79

ejona86 · 2024-09-11T22:58:34Z

xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java


    private void handleClusterDiscoveryError(Status error) {
+      String description = error.getDescription() == null ? "" : error.getDescription() + " ";
+      Status errorWithNodeId = Status.fromCode(error.getCode())


Just use error.withDescription(). That will replace the existing description without modifying the other parts. (So you don't need to fiddle with code and cause)

I thought of using error.augmentDescription but didn't realize this method existed. Thanks for pointing it out, updated to use error.withDescription instead.

ejona86 · 2024-09-11T22:58:59Z

xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java

    }

    private void handleClusterDiscoveryError(Status error) {
+      String description = error.getDescription() == null ? "" : error.getDescription() + " ";


This will have two spaces before " xDS node ID" if getDescription() != null.

When description == null, then there will be a one space indent before "xDS node ID", so I'd leave the extra space here and remove it from the string building.

As Larry suggested, removed extra space from string building in L294.

ejona86 · 2024-09-11T22:59:53Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

      // failure information for addresses yet still providing a service config, the config seector
      // could be avoided.
+      String errorWithNodeId =
+          error + "xDS node ID: " + xdsClient.getBootstrapInfo().node().getId();


This will run together with error. There's not even a space separating them.

Should use ", " to separate them.

ejona86 · 2024-09-11T23:13:34Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

          return;
        }
-        String error = "RDS resource does not exist: " + resourceName;
+        String error = "RDS resource does not exist: " + resourceName + " xDS node ID: "


This will have two node IDs included, one here and one in cleanUpRoutes().

Removed xDS node ID from here.

larry-safran · 2024-09-12T00:29:20Z

xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java

                  "CDS error: circular aggregate clusters directly under %s for "
-                      + "root cluster %s, named %s",
-                  clusterState.name, root.name, namesCausingLoops));
+                      + "root cluster %s, named %s xDS node ID: %s",


Add a comma between named and XDS node ID

larry-safran · 2024-09-12T00:31:56Z

xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java

        Status unavailable =
-            Status.UNAVAILABLE.withDescription("CDS error: found 0 leaf (logical DNS or EDS) "
-                + "clusters for root cluster " + root.name);
+            Status.UNAVAILABLE.withDescription(


As long as you're updating it, it would be nice to switch to String.format() instead of concatenation

Make sense. Updated to use String.format().

larry-safran · 2024-09-12T00:36:16Z

xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java

    }

    private void handleClusterDiscoveryError(Status error) {
+      String description = error.getDescription() == null ? "" : error.getDescription() + " ";


When description == null, then there will be a one space indent before "xDS node ID", so I'd leave the extra space here and remove it from the string building.

larry-safran · 2024-09-12T00:37:23Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

      // failure information for addresses yet still providing a service config, the config seector
      // could be avoided.
+      String errorWithNodeId =
+          error + "xDS node ID: " + xdsClient.getBootstrapInfo().node().getId();


Should use ", " to separate them.

…iew comments

ejona86 · 2024-09-12T21:43:51Z

xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java

        "CDS error: circular aggregate clusters directly under cluster-02.googleapis.com for root"
            + " cluster cluster-foo.googleapis.com, named [cluster-01.googleapis.com,"
-            + " cluster-02.googleapis.com]");
+            + " cluster-02.googleapis.com]" + ", xDS node ID: " + NODE_ID);


Combine the two string literals on the same line together. Ditto bellow.

Add xDS node ID in control plane errors

f94ab1c

DNVindhya requested review from ejona86 and larry-safran September 11, 2024 22:33

ejona86 reviewed Sep 11, 2024

View reviewed changes

larry-safran approved these changes Sep 12, 2024

View reviewed changes

Updated status descriptions using withDescription() and addressed rev…

6813c7d

…iew comments

ejona86 approved these changes Sep 12, 2024

View reviewed changes

combined string literals

cf2a0f1

DNVindhya merged commit f3cf7c3 into grpc:master Sep 12, 2024

DNVindhya deleted the xds-node-id-in-errors branch September 12, 2024 22:40

kannanjgithub pushed a commit to kannanjgithub/grpc-java that referenced this pull request Oct 23, 2024

xds: Add xDS node ID in few control plane errors (grpc#11519)

5180c12

github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2024

Conversation

DNVindhya commented Sep 11, 2024

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants