feat: integrate agentAPI with resources monitoring logic #16438
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,19 +2,54 @@ package agentapi | |
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "errors" | ||
|
|
||
| "cdr.dev/slog" | ||
| "github.com/coder/coder/v2/agent/proto" | ||
| "github.com/coder/coder/v2/coderd/database" | ||
| ) | ||
|
|
||
| type ResourcesMonitoringAPI struct { | ||
| AgentFn func(context.Context) (database.WorkspaceAgent, error) | ||
| Database database.Store | ||
| Log slog.Logger | ||
| } | ||
|
|
||
| func (a *ResourcesMonitoringAPI) GetResourcesMonitoringConfiguration(ctx context.Context, _ *proto.GetResourcesMonitoringConfigurationRequest) (*proto.GetResourcesMonitoringConfigurationResponse, error) { | ||
| agent, err := a.AgentFn(ctx) | ||
|
There was a problem hiding this comment. nit: I think we could avoid using the There was a problem hiding this comment. A bit surprised there's a so simple option , but makes sense and changed - Thanks ! ✅ |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| _, err = a.Database.FetchMemoryResourceMonitorsByAgentID(ctx, agent.ID) | ||
| if err != nil && !errors.Is(err, sql.ErrNoRows) { | ||
defelmnq marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. I'm confused about something, please help me understand: If someone has configured a resources monitor like this: resource "coder_agent" "main" {
arch = data.coder_provisioner.dev.arch
os = data.coder_provisioner.dev.os
resources_monitoring {
memory {
enabled = false <------------------------------
threshold = 80
}
...
}
}It looks like we'll still be collecting the memory datapoints. Ditto for volumes. Is this by design? There was a problem hiding this comment. Yep - I indeed skipped this one on the first proto version which I just changed. New payload seem way better.
|
||
| return nil, err | ||
| } | ||
|
|
||
| volumeMonitors, err := a.Database.FetchVolumesResourceMonitorsByAgentID(ctx, agent.ID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| volumes := make([]string, 0, len(volumeMonitors)) | ||
| for _, monitor := range volumeMonitors { | ||
| volumes = append(volumes, monitor.Path) | ||
| } | ||
|
|
||
| return &proto.GetResourcesMonitoringConfigurationResponse{ | ||
| Enabled: false, | ||
|
There was a problem hiding this comment. It would be useful to reviewers to understand when and how this will be set to There was a problem hiding this comment. You're right - To clarify, this boolean defines if the monitoring is enabled or not - which means are we starting the ticker that will fetch the resources and push it back to coderd. As the agent PR is part of a bigger topic, including another PR worked on by Danielle - for now monitoring should be turned off. Once everything merged and we're ready to deploy, we'll just have to turn this one on with the release of coder/coder. Edit : The logic changed a bit since the globally enabled field has since then been removed as discussed in another topic. |
||
| Config: &proto.GetResourcesMonitoringConfigurationResponse_Config{ | ||
| TickInterval: 10, | ||
| NumDatapoints: 20, | ||
| }, | ||
| MonitoredVolumes: volumes, | ||
| }, nil | ||
| } | ||
|
|
||
| func (a *ResourcesMonitoringAPI) PushResourcesMonitoringUsage(ctx context.Context, req *proto.PushResourcesMonitoringUsageRequest) (*proto.PushResourcesMonitoringUsageResponse, error) { | ||
|
There was a problem hiding this comment. self review : This function is just here as a placeholder - we are fine keeping it this way for now as it will be replaced by @DanielleMaywood PR. |
||
| a.Log.Info(ctx, "resources monitoring usage received", | ||
| slog.F("request", req)) | ||
|
|
||
| return &proto.PushResourcesMonitoringUsageResponse{}, nil | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.