Skip to content

Add protos for supporting debugging information returned by SpiceDB#34

Merged
josephschorr merged 1 commit intoauthzed:mainfrom
josephschorr:debug
Jul 19, 2022
Merged

Add protos for supporting debugging information returned by SpiceDB#34
josephschorr merged 1 commit intoauthzed:mainfrom
josephschorr:debug

Conversation

@josephschorr
Copy link
Copy Markdown
Member

@josephschorr josephschorr commented Jul 18, 2022

First step for authzed/spicedb#696

Comment on lines +43 to +47
// permission_or_relation holds the name of the permission or relation on which the Check was performed.
string permission_or_relation = 2;

// permission_or_relation_type holds information indicating whether it was a permission or relation.
RelationPermissionType permission_or_relation_type = 3;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we think this is better than doing

oneof path {
  option (validate.required) = true;
  string relation = 2;
  string permission = 3;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same difference, I guess. I prefer an enum but this works too

Copy link
Copy Markdown
Member Author

@josephschorr josephschorr Jul 18, 2022

Choose a reason for hiding this comment

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

Updated to just name permission and permission_type

@josephschorr josephschorr force-pushed the debug branch 2 times, most recently from 52d8a84 to b943641 Compare July 18, 2022 22:44
jzelinskie
jzelinskie previously approved these changes Jul 19, 2022
// See the github.com/authzed/authzed-go project for the specific header and footer names.
message DebugInformation {
// check holds debug information about a check request.
CheckDebugTrace check = 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be a oneof to support future types, or do you anticipate potentially returning multiple types of trace metadata?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I anticipated having only one for now, but I could see a case where multiple are returned


// CheckDebugTrace is a recursive trace of the requests made for resolving a CheckPermission
// API call.
message CheckDebugTrace {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this include the (relevant) schema as context too, since that is mutable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea; I'll add it


// result holds the result of the Check call.
Permissionship result = 5;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make sense to skip a few field numbers here to leave space for caveat-related data?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think so; the field number ordering doesn't really matter, especially since this is marshaled to JSON

Copy link
Copy Markdown
Contributor

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

LGTM

@josephschorr josephschorr merged commit 30f821c into authzed:main Jul 19, 2022
@josephschorr josephschorr deleted the debug branch July 19, 2022 20:31
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2022
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