make the tablet identification added to errors more concise#3015
Merged
sougou merged 2 commits intovitessio:masterfrom Aug 2, 2017
Merged
make the tablet identification added to errors more concise#3015sougou merged 2 commits intovitessio:masterfrom
sougou merged 2 commits intovitessio:masterfrom
Conversation
Instead of printing the full Tablet object in every error from vtgate, only include the cell, uid, and hostname to reduce the clutter in error logs. The removed information is still available through the topology if needed for debugging.
alainjobart
reviewed
Jul 26, 2017
Contributor
alainjobart
left a comment
alainjobart
left a comment
There was a problem hiding this comment.
Good change, couple suggested changes :)
| func tabletIdent(tablet *topodatapb.Tablet) string { | ||
| return fmt.Sprintf("%s-%d (%s)", tablet.Alias.Cell, tablet.Alias.Uid, tablet.Hostname) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Let's move that function to go/vt/topotools/tablet.go, export it and name it TabletIdent.
Additionally, it would be nice to also have in there the 'tags' fields, something like:
cell-123 (host1 tag1=value1)
The tags are usually used to describe a tablet (and for instance put some extra name in there, that's what we do inside YouTube). They are displayed in 'vtctl ListAllTablets' and it's very useful.
go/vt/vtgate/gateway/shard_error.go
Outdated
| } | ||
| if tablet != nil { | ||
| return vterrors.Errorf(vterrors.Code(in), "target: %s.%s.%s, used tablet: (%+v), %v", target.Keyspace, target.Shard, topoproto.TabletTypeLString(target.TabletType), tablet, in) | ||
| return vterrors.Errorf(vterrors.Code(in), "target: %s.%s.%s, used tablet: %s-%d (%s), %v", target.Keyspace, target.Shard, topoproto.TabletTypeLString(target.TabletType), tablet.Alias.Cell, tablet.Alias.Uid, tablet.Hostname, in) |
Contributor
There was a problem hiding this comment.
And then you can use TabletIndent here too.
Contributor
|
+1 to this change. I've been meaning to get to this myself |
Based on PR feedback: * Add Tags * Move TabletIdent into topotools Added a simple test as well
Member
Author
|
@alainjobart Thanks for the feedback. Updated as per your suggestions. |
sougou
approved these changes
Aug 2, 2017
This was referenced Jun 24, 2021
This was referenced Aug 4, 2021
This was referenced Jun 20, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of printing the full Tablet object in every error that is propagated from vtgate, include a concise summary of the cell, uid, and hostname to reduce the clutter in error logs.
As an example from the discoverygateway tests, this error:
Would now be:
The removed information is still available through the topology if needed for debugging.