syncv2: Add metric submission support#103
Conversation
|
The latest Buf updates on your PR. Results from workflow .github/workflows/update.yml / generate (pull_request).
|
📝 WalkthroughWalkthroughAdds a new PublishMetrics RPC to the SantaSync protobuf API and introduces request/response messages for sending typed metrics, root labels, and a machine identifier. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@syncv2/v2.proto`:
- Around line 1268-1273: The MetricKind enum must have UNKNOWN = 0 as the first
entry; update the enum declaration for MetricKind so the first value is
MetricKind_UNKNOWN = 0 and renumber the remaining entries consecutively (e.g.,
move MetricKind_UNSPECIFIED to 1 and increment MetricKind_CONSTANT,
MetricKind_GAUGE, MetricKind_COUNTER accordingly) so values remain unique and
sequential.
- Around line 57-62: The RPC definition for PublishMetrics (rpc
PublishMetrics(PublishMetricsRequest) returns (PublishMetricsResponse)) ends
with a trailing semicolon after the closing brace; remove the semicolon so the
RPC block ends with just "}" to comply with proto3 service syntax and ensure the
PublishMetrics RPC (and its http option block with post "/metrics/{machine_id}"
and body "*") parses correctly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
syncv2/v2.proto (1)
1287-1290: Rename oneof fields to non-keyword identifiers.Using
int64 int64,bool bool,string string, anddouble doubleas field names is syntactically valid but fragile across proto tooling. Protobuf explicitly recommends avoiding type-name and keyword identifiers, as code generators across languages (Rust, Java, Go, etc.) may escape, rename, or trigger conflict-resolution suffixing for accessor methods—potentially breaking reflection and dynamic tooling. Use explicit names likeint64_value,bool_value,string_value,double_value.Proposed diff
oneof metric_value { - int64 int64 = 2; - bool bool = 3; - string string = 4; - double double = 5; + int64 int64_value = 2; + bool bool_value = 3; + string string_value = 4; + double double_value = 5; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@syncv2/v2.proto` around lines 1287 - 1290, The oneof fields currently named "int64", "bool", "string", and "double" should be renamed to non-keyword identifiers to avoid generator conflicts; update the field names to "int64_value", "bool_value", "string_value", and "double_value" in the proto (the message/oneof that contains the lines "int64 int64 = 2;", "bool bool = 3;", "string string = 4;", "double double = 5;") and then update any references/usages of those fields across the codebase or regenerate language stubs so callers use the new accessors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@syncv2/v2.proto`:
- Around line 1287-1290: The oneof fields currently named "int64", "bool",
"string", and "double" should be renamed to non-keyword identifiers to avoid
generator conflicts; update the field names to "int64_value", "bool_value",
"string_value", and "double_value" in the proto (the message/oneof that contains
the lines "int64 int64 = 2;", "bool bool = 3;", "string string = 4;", "double
double = 5;") and then update any references/usages of those fields across the
codebase or regenerate language stubs so callers use the new accessors.
Part of SNT-14