Add discoveryServiceMapEnabled field to AgentConfiguration proto#471
Add discoveryServiceMapEnabled field to AgentConfiguration proto#471
Conversation
BarFinsdd
force-pushed
the
bar.fins/discovery-dep-map-poc
branch
from
ac0587c to
0233288
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BarFinsdd
force-pushed
the
bar.fins/discovery-dep-map-poc
branch
from
0233288 to
c9b279f
Compare
brycekahle
left a comment
brycekahle
left a comment
There was a problem hiding this comment.
It seems fine, but I'm curious if you need a flag at all? Is there going to be additional data in the payload? Would the existence of that data indicate the feature is enabled?
Discovery service map mode does not retain DDSketches or per-request samples; it tracks only counts and a running latency sum. Add a dedicated avgLatency field (nanoseconds) so downstream consumers can distinguish discovery-mode averages from the legacy firstLatencySample single-sample optimization. Field 5 on HTTPStats.Data, double, fixed64. Generated code edited by hand (proto codegen toolchain isn't available on Windows). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76133a4c54
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Populated by discovery service map mode in lieu of DDSketches and | ||
| // firstLatencySample, since discovery mode does not retain a sketch | ||
| // or per-request samples. | ||
| AvgLatency float64 `protobuf:"fixed64,5,opt,name=avgLatency,proto3" json:"avgLatency,omitempty"` |
There was a problem hiding this comment.
Adding AvgLatency here without updating the embedded fileDescriptor_69b34851fbf62631 leaves HTTPStats_Data.Descriptor() out of sync with the struct and marshal/unmarshal logic. Any reflection-based consumer (e.g., descriptor-driven tooling, dynamic protobuf handling, or schema introspection) will not see avgLatency and can silently ignore/drop it even though generated encode/decode methods now use field 5; this commit should include a full proto regeneration so descriptor metadata matches the new field.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
What does this PR do?
Adds a new discoveryServiceMapEnabled (field 7) boolean to the AgentConfiguration proto message in process/connections.proto. This allows the agent to signal whether the Discovery service map feature is enabled in its
configuration.
Motivation
Enables the agent to communicate whether the Discovery dependency map (service map) feature is turned on, so downstream consumers (e.g. the backend/process-agent) can act on that configuration flag. This is the proto
groundwork for the Discovery service map POC.
Additional Notes
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Reviewer's Checklist
Reviewers: please see the review guidelines.