-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: opamp implementation for nodejs agent #1287
Conversation
nit: maybe we should put the files in |
} | ||
} | ||
|
||
func (k *K8sPodInfoResolver) getServiceNameFromAnnotation(ctx context.Context, name string, kind string, namespace string) (string, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these functions should idealy be under k8sutils and not opampserver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will handle in a followup PR
opampserver/protobufs/anyvalue.pb.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these generated with protoc
, if so maybe worth adding in the README how to generate them,
Are we using the proto defined in opAmp, or are we having some custom types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instructions can be found here: https://github.com/odigos-io/opamp-spec
// unfortunately, I did not find a way to get just one device id from the kubelet, we need | ||
// to list and iterate over all the devices to get a current snapshot of allocations on this node | ||
// and then look for the ones we are interested in. | ||
func (c *KubeletClient) DeviceIdsToContainerDetails() (map[string]*ContainerDetails, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also does not look like it relates specifically to opamp server, maybe worth moving to k8sutils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I suggest doing it later on
}); | ||
} | ||
|
||
private async sendHeartBeatToServer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the heartbeat to keep the connection alive or is it part of OpAMP spec? Maybe we can drop it if we are not doing anything on the heartbeat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpAMP server uses it to figure out if the process (or the connection) is down
No description provided.