feat: opamp implementation for nodejs agent #1287
Conversation
|
nit: maybe we should put the files in |
| "compile": "tsc -p ." | ||
| }, | ||
| "dependencies": { | ||
| "@bufbuild/protobuf": "^1.10.0", |
There was a problem hiding this comment.
Update path in dependabot.yaml
| @@ -3,10 +3,15 @@ WORKDIR /python-instrumentation | |||
| ADD odiglet/agents/python/requirements.txt . | |||
| RUN mkdir workspace && pip install --target workspace -r requirements.txt | |||
|
|
|||
| FROM node:16 AS nodejs-builder | |||
| FROM node:18 AS nodejs-builder | |||
| ARG ODIGOS_VERSION | |||
There was a problem hiding this comment.
Not directly connected to this PR, but can we use a similar approach for the go code to inject the version? would that make it easier compared to what we do today?
There was a problem hiding this comment.
Currently. I think we should do it for all agents 馃憤
|
|
||
| func StartOpAmpServer(ctx context.Context, logger logr.Logger, mgr ctrl.Manager, kubeClient *kubernetes.Clientset, nodeName string) error { | ||
|
|
||
| listenEndpoint := "0.0.0.0:4320" |
|
|
||
| http.HandleFunc("/v1/opamp", func(w http.ResponseWriter, req *http.Request) { | ||
| // Check for the correct method, e.g., GET | ||
| if req.Method != "POST" { |
There was a problem hiding this comment.
since go 1.22 this is not required, you can do
http.HandleFunc("POST /v1/opamp"...
| return "", false | ||
| } | ||
|
|
||
| overwrittenName, exists := annotations["odigos.io/reported-name"] |
There was a problem hiding this comment.
consts.OdigosReportedNameAnnotation
| } | ||
| } | ||
|
|
||
| func (k *K8sPodInfoResolver) getServiceNameFromAnnotation(ctx context.Context, name string, kind string, namespace string) (string, bool) { |
There was a problem hiding this comment.
I think these functions should idealy be under k8sutils and not opampserver
There was a problem hiding this comment.
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?
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| logger.Info("Shutting down live connections timeout monitor") |
There was a problem hiding this comment.
Is there any cleanup we need to do here in exit?
| // 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.
This also does not look like it relates specifically to opamp server, maybe worth moving to k8sutils
| private resourcePromiseResolver?: (resourceAttributes: Attributes) => void; | ||
|
|
||
| constructor(config: OpAMPClientHttpConfig) { | ||
| this.config = config; |
There was a problem hiding this comment.
Should we put a timeout on the HTTP requests?
| baseURL: `http://${this.config.opAMPServerHost}`, | ||
| headers: { | ||
| "Content-Type": " application/x-protobuf", | ||
| Authorization: `DeviceId ${config.instrumentationDeviceId}`, |
There was a problem hiding this comment.
I think calling the header Authorization is a little misleading.
This gives a sense of secure connection but obviously, it is very easy to manipulate. Maybe something like X-Odigos-DeviceId
| }); | ||
| } | ||
|
|
||
| private async sendHeartBeatToServer() { |
There was a problem hiding this comment.
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
No description provided.