-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add QueryStats and QueryMetadata to TraceItemTableResponse #51
base: main
Are you sure you want to change the base?
Conversation
- Update proto/sentry_protos/snuba/v1/endpoint_trace_item_table.proto to include optional QueryStats and QueryMetadata fields in TraceItemTableResponse - Modify proto/sentry_protos/snuba/v1/request_common.proto to define QueryStats and QueryMetadata message structures These changes allow for more detailed query execution information to be included in responses, enhancing observability and debugging capabilities.
@@ -21,6 +21,7 @@ message TraceItemTableRequest { | |||
uint32 limit = 6; | |||
PageToken page_token = 7; | |||
repeated VirtualColumnContext virtual_column_contexts = 8; | |||
bool debug = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug flag will be used to add QueryStats and QueryMetadata in the response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go in the RequestMeta
object so it can be generally available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's better for our next version of the API to have a ResponseMeta
, similar to the RequestMeta
that every endpoint returns. That would make it easier to have these common return objects in every endpoint.
@@ -39,4 +40,6 @@ message TraceItemColumnValues { | |||
message TraceItemTableResponse { | |||
repeated TraceItemColumnValues column_values = 1; | |||
PageToken page_token = 2; | |||
optional QueryStats query_stats = 3; | |||
optional QueryMetadata query_metadata = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put both of these objects inside another object, so that you only have to add one field to the response instead of two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should also be repeated because there could be many queries in one request
@@ -21,6 +21,7 @@ message TraceItemTableRequest { | |||
uint32 limit = 6; | |||
PageToken page_token = 7; | |||
repeated VirtualColumnContext virtual_column_contexts = 8; | |||
bool debug = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go in the RequestMeta
object so it can be generally available.
Add QueryStats and QueryMetadata to TraceItemTableResponse