Conversation
|
The latest Buf updates on your PR. Results from workflow ci / buf-checks (pull_request).
|
There was a problem hiding this comment.
Pull request overview
Adds an explicit source-column type to VirtualColumnContext in the Snuba v1 trace item attribute proto to support more flexible virtual column mappings (EAP-462).
Changes:
- Introduces
from_column_type(AttributeKey.Type) onVirtualColumnContext. - Adds a brief comment describing the design alternative considered.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| map<string, string> value_map = 3; | ||
| string default_value = 4; | ||
| // Alternatively, I could just use AttributeKey and deprecate from_column_name, but it might be confusing for the caller | ||
| AttributeKey.Type from_column_type = 5; |
There was a problem hiding this comment.
In proto3, a non-optional enum field has no presence tracking; when unset it will be read as TYPE_UNSPECIFIED (0). If TYPE_UNSPECIFIED is not a valid/meaningful value here, consider making from_column_type optional and/or explicitly documenting the default behavior when it is omitted.
| AttributeKey.Type from_column_type = 5; | |
| optional AttributeKey.Type from_column_type = 5; |
| map<string, string> value_map = 3; | ||
| string default_value = 4; | ||
| // Alternatively, I could just use AttributeKey and deprecate from_column_name, but it might be confusing for the caller | ||
| AttributeKey.Type from_column_type = 5; |
There was a problem hiding this comment.
This repo checks in generated Rust bindings (e.g. rust/src/sentry_protos.snuba.v1.rs currently defines VirtualColumnContext without from_column_type). Please regenerate/update the checked-in generated code (at least rust/src/sentry_protos.snuba.v1.rs) so Rust consumers can access the new field and CI/codegen stays consistent.
| string to_column_name = 2; | ||
| map<string, string> value_map = 3; | ||
| string default_value = 4; | ||
| // Alternatively, I could just use AttributeKey and deprecate from_column_name, but it might be confusing for the caller |
There was a problem hiding this comment.
The inline comment is written in first person and describes an implementation alternative rather than documenting the API contract. Please replace it with a caller-focused comment that explains when/why to set from_column_type and how it interacts with from_column_name (and remove the editorial note).
| // Alternatively, I could just use AttributeKey and deprecate from_column_name, but it might be confusing for the caller | |
| // Optional explicit type of the source column referenced by from_column_name. | |
| // Set this when the type of from_column_name cannot be inferred or needs to be | |
| // specified for correct interpretation of value_map entries. Together, | |
| // from_column_name and from_column_type describe the source values that are | |
| // mapped to the virtual column identified by to_column_name. |
|
@xurui-c maybe i don't have a great understanding here but why do we not want to update |
|
what if vcc wants to support a column that isn't a normalized column? i wouldn't want to add it every time. But, let me know if vcc is intended to only work with normalized columns @MeredithAnya |
EAP-462