-
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: nodejs opamp instrumentation libraries enabled remote config #1350
Conversation
helm chart: odigos-io/odigos-charts#99 |
connCopy := *conn | ||
c.mux.Lock() | ||
defer c.mux.Unlock() | ||
c.liveConnections[deviceId] = &connCopy |
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 remember we discussed about an option to save {deviceId}-{InstanceId} to handle fork cases?
opampserver/pkg/server/handlers.go
Outdated
|
||
sdkConfig, err := json.Marshal(RemoteConfigSdk{RemoteResourceAttributes: serverOfferedResourceAttributes}) | ||
fullRemoteConfig, err := c.sdkConfig.GetFullConfig(ctx, remoteResourceAttributes, &podWorkload) |
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.
We can maybe send the instrumentedAppName to the GetFullConfig so we wont need to call GetRuntimeObjectName again in it?
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.
great catch. fixed 👍
Pid: pid, | ||
InstrumentedAppName: instrumentedAppName, | ||
AgentRemoteConfig: fullRemoteConfig, | ||
RemoteResourceAttributes: remoteResourceAttributes, |
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.
as i understood it, the remoteResourceAttributes
is part of the fullRemoteConfig, why we need it in the ConnectionInfo as well?
I saw that we save it in the connectionCache and never use it from the cache
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.
We use it when one of the other reconcilers updates and we need to calculate the entire full config from scratch. If we don't store it anywhere, we will need to calculate on each new config update.
I agree it's not that clear right now, would love to address it oneday and improvre
Amir, what do you think about aligning the opamp server response/request with the terminology of |
good point. updated 👍 |
No description provided.