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.

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

xds: error descriptions #8554

Merged
merged 5 commits into from
Sep 24, 2021
Merged

xds: error descriptions #8554

merged 5 commits into from
Sep 24, 2021

Conversation

YifeiZhuang
Copy link
Contributor

@YifeiZhuang YifeiZhuang commented Sep 23, 2021

  • audited existing error messages in xds name resolver, xds lbs, xdsServerWrapper and made changes to add more error description verbosely. However, we should discuss how much error details of internal processing logics we should expose to messages visible to customers, e.g.
    • what is the immediately/root cause of the error: what components, what config is missing, what is invalid, what is expected, or unknown errors?
    • hint what is the possible action to fix the error?
  • added more debug logs in xds server

@YifeiZhuang YifeiZhuang marked this pull request as draft September 23, 2021 00:37
@YifeiZhuang YifeiZhuang marked this pull request as ready for review September 23, 2021 20:12
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

This seems about the right level of verbosity. We want to give the essential context without drowning the user in data. Generally, we should say what is broken and not try to assume how it became broken. That also means generally not suggesting how to fix the error (but there are sometimes special cases to that).

On server-side we'll need to use more logs than status codes, since we shouldn't be sharing too much information with the client.

xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java Outdated Show resolved Hide resolved
@@ -281,6 +281,9 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
SslContextProviderSupplier sslContextProviderSupplier = InternalProtocolNegotiationEvent
.getAttributes(pne).get(ATTR_SERVER_SSL_CONTEXT_PROVIDER_SUPPLIER);
if (sslContextProviderSupplier == null) {
logger.log(Level.INFO, "No sslContextProviderSupplier found in filterChainMatch "
Copy link
Member

Choose a reason for hiding this comment

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

This may be normal. INFO is logged by default, and we don't want to log by default when nothing is wrong. Use FINE level?

Ugh. Looks like the "Using fallback" log also needs fixing.

Copy link
Contributor

Choose a reason for hiding this comment

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

And when fallback exists I think it will be nice to have a single message that includes "no ... found and using fallback for ...".

Also do we need to print out variable names like sslContextProviderSupplier or can we just say no filter-matched for connection ..."?

Copy link
Member

Choose a reason for hiding this comment

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

It's not an error, so no log. Everything is working as expected. We also have to be careful on server-side not to log too frequently; if every connection fails the same way it can produce a lot of logs very quickly.

@YifeiZhuang YifeiZhuang merged commit 0245a72 into grpc:master Sep 24, 2021
@YifeiZhuang YifeiZhuang deleted the xds_msg branch September 24, 2021 17:36
ejona86 added a commit to ejona86/grpc-java that referenced this pull request Sep 24, 2021
The sslContextProviderSupplier is used by the xds creds themselves when
the control plane has security configured. But the fallback credentials
don't use such a supplier and may not even be using TLS.

Language tweak following grpc#8554.
ejona86 added a commit that referenced this pull request Sep 24, 2021
The sslContextProviderSupplier is used by the xds creds themselves when
the control plane has security configured. But the fallback credentials
don't use such a supplier and may not even be using TLS.

Language tweak following #8554.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants