feat: add system metrics enabled config #163
Conversation
|
Static check's been failing here too @tim-mwangi :| |
I wonder if it's a permissions thing. I have added @thugrock7 to the collaborators with Write access. I will do the same for the goagent repo. It's better if @thugrock7 asked to be added to the Hypertrace organization. |
|
|
||
| // SystemMetrics has the config for capturing system metrics. | ||
| message SystemMetrics { | ||
| // Whether to capture system metrics or not. The system metrics is setup in goagent. |
There was a problem hiding this comment.
nit: update the comment to Whether to capture system metrics or not. The system metrics is setup in goagent. The system metrics is only supported in goagent as of now
There was a problem hiding this comment.
Whether to capture system metrics or not. The system metrics are setup in each agent
We do not need to add that it's only supported in goagent, because we can update any agent at any time. With this config, any agent can use it.
We can keep this as part of this PR that during the time of this PR, we were only adding support for goagent.
There was a problem hiding this comment.
You're right, just wanted to propagate the idea that support needs to be added explicitly
There was a problem hiding this comment.
LGTM, pls wait for @tim-mwangi 's or @ryanericson 's approval
| message Metrics { | ||
| // Whether to capture metrics or not. The metrics will be otel go metrics. | ||
| // See https://github.com/open-telemetry/opentelemetry-go/tree/main/metric | ||
| google.protobuf.BoolValue enabled = 1; |
There was a problem hiding this comment.
So this is top level enable disable all metrics right?
And system_metrics is for cpu and memory only? Do we need a separate flag for system metrics though? I wouldn't think it's very risky? I suppose we've had it on by default for ebpf right?
There was a problem hiding this comment.
Yes, for ebpf we have it on by default. Was wondering from a generic metrics config point of view, if we need to make it modular (coming from our discussion here).
There was a problem hiding this comment.
But that ship has already sailed as the levels of this config is different in both the places. Maybe we can merge both of these protos into one and make a single config, but ig that'd require the hypertrace agents to be deprecated :|
There was a problem hiding this comment.
In that case I think it can all just be controlled by this single flag then. So no need for this change.
|
closing this pr as mentioned here, #163 (comment) |
Description
Config to enable system metrics. These system metrics are setup in goagent
Linked Issue: hypertrace/goagent#235