Conversation
xrmx
force-pushed
the
opamp-prototype
branch
3 times, most recently
from
d2691b2 to
4f0b176
Compare
While at it remove unused client method
v1v
left a comment
v1v
left a comment
There was a problem hiding this comment.
LGTM - I only reviewed the .github/workflows folder
| _HANDLED_CAPABILITIES = ( | ||
| opamp_pb2.AgentCapabilities.AgentCapabilities_ReportsStatus | ||
| | opamp_pb2.AgentCapabilities.AgentCapabilities_ReportsHeartbeat | ||
| | opamp_pb2.AgentCapabilities.AgentCapabilities_AcceptsRemoteConfig |
There was a problem hiding this comment.
Just for discussion: I guess this implementation doesn't report the status of given remote config (this is related to the ReportsRemoteConfig agent capability)?
There was a problem hiding this comment.
Yep, for now the server will always report the full config and we're going to act on that at every heartbeat. Will work on that later though but I need to introduce the concept of a config that we don't have in the python sdk atm.
There was a problem hiding this comment.
Not sure if we are talking about different things. I mean sending an AgentToServer message with remote_config_status set.
For example, here is one place in EDOT Node.js's onRemoteConfig handler function that calls the OpAMP client's method to send this "remote_config_status":
https://github.com/elastic/elastic-otel-node/blob/682bdc2444e9da6a25adb2ea2d03fd11cfe89783/packages/opentelemetry-node/lib/central-config.js#L165-L168
We don't really have a concept of a "config" in the Node.js SDK either. There is no state on the SDK outside of the onRemoteConfig() function to make this response.
Anyway, it isn't a requirement to be sending this, so all good.
There was a problem hiding this comment.
I'll add this in a followup PR
| yield config_file_name, config_data | ||
| else: | ||
| raise OpAMPRemoteConfigParseException( | ||
| f"Cannot parse {config_file_name} with content type {config_file.content_type}" |
There was a problem hiding this comment.
nit: Not sure if I read this correctly, but if the OpAMP server returns any config file with a content-type other than one of the two JSON ones, we'll throw here, and eventually get:
logger.warning("Job %r handler failed with: %s", job.payload, exc)There was a problem hiding this comment.
Yes, do you suggest to ignore errors on not elastic filenames? I don't expect any though
There was a problem hiding this comment.
I don't expect any though
I called this a "nit" because I'm hunting for edge cases that are unlikely.
do you suggest to ignore errors on not elastic filenames?
That's what my implementation is doing:
https://github.com/elastic/elastic-otel-node/blob/682bdc2444e9da6a25adb2ea2d03fd11cfe89783/packages/opentelemetry-node/lib/central-config.js#L89-L102
It only ever looks at the elastic entry in the configMap.
There was a problem hiding this comment.
I'll try to upstream the client so I cannot add elastic specific cases when decoding messages, if that would become a problem in practice I can revise it later I guess.
5 participants
Introduce a basic OpAMP agent in our distro that permits to update the current config at runtime. At the moment only the
logging_levelconfig is supported to update dynamically the sdk logging level.Fixes #253