Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds explicit agent “node” (OS/platform) metadata to enable generating Windows-specific telemetry configuration, alongside updating the public API and proto build workflow.
Changes:
- Add
nodeto the Agent model/controller and expose it via gRPC/protobuf. - Extend Alloy config generation to support Windows node exporter and Windows Event Log sources.
- Simplify
make prototo runprotocdirectly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/model/agent.go | Persist node on agents (defaulting to unix at DB level). |
| internal/grpc/server.go | Plumb node through Register/Get agent RPCs. |
| internal/controller/agent.go | Extend controller Agent input with node. |
| internal/controller/agent_marshal.go | Marshal/persist node; allow event log source scheme. |
| internal/controller/agent_config.go | Generate Windows-specific Alloy config blocks (eventlog + windows exporter). |
| api/api.proto | Add node to RegisterAgentRequest and GetAgentResponse messages. |
| api/api.pb.go | Regenerated protobuf bindings reflecting the new fields. |
| Makefile | Update proto target to run protoc without the prior wrapper check. |
Comments suppressed due to low confidence (2)
internal/controller/agent_marshal.go:37
Nodeis persisted exactly as provided. Since callers in the codebase often constructcontroller.Agent{Hostname, LogSources}without settingNode, this will store an empty node value and later templates will not match either "unix" or "windows". DefaultNodeto "unix" when empty and validate/normalize supported values before creating the model agent.
agent := &model.Agent{
Hostname: data.Hostname,
Node: data.Node,
LogSources: effectiveLogSources,
Metrics: data.Metrics,
MetricsTargets: effectiveMetricsTargets,
Profiles: data.Profiles,
Labels: data.Labels,
ResourceId: fmt.Sprintf("rid:finch:%s:agent:%s", c.config.Id(), uuid.New().String()),
}
internal/controller/agent_marshal.go:75
eventlog sources are accepted solely based on URI scheme. This allows values likeevent://orevent:that parse successfully but provide no event log name, which later renderseventlog_name = ""and produces an invalid Alloy config. Require a non-empty event log name (e.g.,uri.Hostoruri.Opaque) whenuri.Scheme == "event", and consider returning a validation error rather than silently skipping/accepting.
uri, err := url.Parse(logSource)
if err != nil {
continue
}
if !slices.Contains([]string{"journal", "docker", "file", "event"}, uri.Scheme) {
continue
}
effectiveLogSources = append(effectiveLogSources, uri.String())
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
84f319f to
c5e59fa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/controller/agent_marshal.go:78
- Consider adding validation to ensure log source types are compatible with the node type. For example, 'event' log sources should only be allowed for Windows nodes, while 'journal' and 'docker' sources are typically Unix-specific. Currently, the code accepts any combination, which could result in invalid Alloy configurations that fail at runtime on the agent. Adding this validation in marshalNewAgent would provide earlier feedback to API consumers.
func (c *Controller) __parseLogSources(data *Agent) ([]string, error) {
if len(data.LogSources) == 0 {
return nil, fmt.Errorf("at least one log source must be specified")
}
var effectiveLogSources []string
for _, logSource := range data.LogSources {
uri, err := url.Parse(logSource)
if err != nil {
continue
}
if !slices.Contains([]string{"journal", "docker", "file", "event"}, uri.Scheme) {
continue
}
effectiveLogSources = append(effectiveLogSources, uri.String())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c5e59fa to
9247c4d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.