grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

Published 20 hours ago • verified publisher icon grpc

internal/xdsclient: xdsclient transport logs should indicate the authority

Open my4-dev opened this issue 2 years ago • 7 comments

For more information, please refer to #6091 .

RELEASE NOTES: none

my4-dev avatar Apr 23 '23 08:04 my4-dev

Optional: While you are there, it would be great if you could you also help cleanup these incorrect usages of logger

The write way would be by calling the xdsclient's logger -- which means you could get away with a simple

- logger.Warningf("Watch registered for name %q of type %q which is already registered", rType.TypeName(), resourceName) 
+ c.logger.Warningf("Watch registered for name %q of type %q which is already registered", rType.TypeName(), resourceName)

arvindbr8 avatar Apr 26 '23 16:04 arvindbr8

Hi, @arvindbr8 ! Thanks for your advice! I have some opinions to the approach you suggested earlier.

  1. internalgrpclog.NewPrefixLogger's first argument type is DepthLoggerV2, so we couldn't use NewPrefixLogger like you suggested without changing this API.
  2. I have added the usage of prefixLoggerAuthorityArgs at newAuthority method in authoriy.go because we want to confirm the information of management servers whenever we initiate an authority.

my4-dev avatar May 08 '23 13:05 my4-dev

SG. Let me know when you want me to take another look at it.

arvindbr8 avatar May 10 '23 16:05 arvindbr8

Assigning it back to you @my4-dev. Seems like this might still be in progress. Let me know if you still have questions.

arvindbr8 avatar May 16 '23 21:05 arvindbr8

@arvindbr8 : Thank you for assigning back. I don't have any questions. If you or other member don't have any other suggestions or opinions after reviewing this PR, it would be ready to be merged.

my4-dev avatar May 17 '23 11:05 my4-dev

sure. taking another look

arvindbr8 avatar May 23 '23 00:05 arvindbr8

Hi, @arvindbr8 !

The diff i suggested in my previous comment seems to checkout in my local. Is that a type mismatch error that you see?

I'm sorry, I tried again and could confirm that a type mismatch error didn't occur. I might have tried with c.logger instead of logger🙏 I think that it is unnecessary to expose a new API (AddPrefix) to the preflixLogger too.

All the information required about the mgmt server should be available to you from the bootstrap config.

If we forget setting logger appropriately when we initiate authority by using newAuthority in other situation in the future, mgmt server information don't be outputted by logger. Do we need to consider this situation?

my4-dev avatar May 29 '23 12:05 my4-dev

Sorry for the delay. I was out of office. Taking a look now.

arvindbr8 avatar Jun 20 '23 15:06 arvindbr8

I dont think adding another prefix to our prefix logger would be the right way to go. Let me clone your repo and check the type error from my side.

arvindbr8 avatar Jun 20 '23 16:06 arvindbr8

Hi @my4-dev, Could you check this diff out. My idea to implement this would be along this diff.

On a high level, the logger that is passed to authority.go and transport.go needs to have the new prefix as per #6091. I dont believe adding a AddPrefix method is the right way to go about it. Please take a look and let me know if this PR can align more on those lines.

If you think its too much work, feel free to discard this PR and one of us could definitely take this up.

arvindbr8 avatar Jun 27 '23 22:06 arvindbr8

@arvindbr8 : I'm sorry, I want one of your team members to take this up🙇‍♂️

my4-dev avatar Jun 30 '23 12:06 my4-dev