First pass spec for taking thread dumps - #403
Conversation
| opamp/development: | ||
| endpoint: http://some.opamp-host.com:3420/v1/opamp | ||
| experimental_control: true |
There was a problem hiding this comment.
| opamp/development: | |
| endpoint: http://some.opamp-host.com:3420/v1/opamp | |
| experimental_control: true | |
| opamp/development: | |
| endpoint: http://some.opamp-host.com:3420/v1/opamp | |
| features: | |
| remote_control: |
There was a problem hiding this comment.
Oh yeah, this declarative config stuff hates explicit feature toggles and prefers empty blocks to do the same thing. Ugh. Regardless, I think it very much needs the word experimental in there -- so that it sends a clear message to the user.
There was a problem hiding this comment.
Could it be experimental_remote_control: or remote_control/development: ?
MrAlias
left a comment
MrAlias
left a comment
There was a problem hiding this comment.
Thanks for carrying the compatibility bridge through. I think we need a few contract details resolved before we merge it: the present wording can bypass the opt-in and allow duplicate or unexpected command execution. Once those are nailed down, it should give the Java implementation—and future agents—a dependable target.
| `AgentConfigFile` whose body is UTF-8 encoded. Its `content_type` SHOULD be | ||
| `text/plain; charset=utf-8`. | ||
|
|
||
| When `COMMAND_HACKS` is present, the agent MUST interpret its body as a command |
There was a problem hiding this comment.
Could we make handling COMMAND_HACKS conditional on the experimental-control option, and say agents advertise the command capability only when that option is enabled? As written, this unconditional MUST applies to an agent with ordinary remote configuration enabled but remote control still at its default of false; receiving this filename would execute a command anyway.
|
|
||
| The first thread dump is collected immediately. When `count` is greater than | ||
| one, the agent waits `interval_millis` after completing one thread dump before | ||
| starting the next. The agent MUST use the command's `job_id` for every thread |
There was a problem hiding this comment.
Could we define duplicate-delivery semantics here? OpAMP can repeat remote-configuration content after reconnects or status reports, and job_id currently appears only in the output labels. A bounded in-memory record of accepted or completed job IDs, together with defined accepted, rejected, and completed outcomes, would keep a resend from collecting the same sequence again.
There was a problem hiding this comment.
Although the thing you're describing definitely improves robustness, I would prefer to not have that level of completeness in the spec at this very early stage. I think that the impact of a potential/possible resend is manageable.
|
|
||
| | Field | Value | | ||
| |--------------|--------------------------------------------| | ||
| | `capability` | `com.splunk.opamp.experimental_command/v1` | |
There was a problem hiding this comment.
This is a very general capability. It is fine solution for the urgent release, but did you also consider using com.splunk.opamp.experimental_thread_dump/v1 instead?
What I'm suggesting is to advertise supported commands in a granular way, where each supported command has its own capability, its own version and data format.
In the future we will probably support more commands and agent would be able to "handshake" with a server in a stricter way. It would also simplify independent developing of new commands and evolving existing.
There was a problem hiding this comment.
This sounds like an entirely reasonable and pragmatic thing to suggest. It also sounds more complicated and possibly over-engineered for what we're doing right now.
My preference would be for the agents to not receive any commands whatsoever, and for all of this to not even exist. With that, I'm not inclined to try and design a comprehensive, future-proof, extensible solution right now. Just trying to do the basic thing.
I don't think that having additional finer-grained capabilities defined later, even if they too were "commands" or whatever. In fact, if. this stupid thing sticks around and we have to support it long term, that might be a road toward "stability" -- so we keep the "experimental" umbrella for all commands not stable, and the path to stabilizing a given command is to have it as a first-class versioned capability. I dunno. I didn't set out to do that, but maybe that's also reasonable?
There was a problem hiding this comment.
Ok. Let's see what future brings us
| The `thread.dump` command, requests one or more thread dumps of the agent | ||
| process. Its body has the following format: | ||
|
|
||
| ```text |
There was a problem hiding this comment.
What about yaml format here? We use it in many places already.
There was a problem hiding this comment.
just trying to keep things simple and not over-engineer a feature I hope goes away anyway. 🤷🏻
There was a problem hiding this comment.
Well, once something is released it takes years to get rid of it...
And I'm afraid that changing format will be equally hard.
There was a problem hiding this comment.
Nah, we can leverage content-type if/when that's needed.
robsunday
left a comment
robsunday
left a comment
There was a problem hiding this comment.
Looks good for now, with one small question
|
Sounds like we're backing down from this strategy for now, in favor of some profiling enhancements. |
4 participants
We are being requested to make this quick hack to cause thread dumps to be taken and reported via the profiling signal. I have a number of notable concerns with this, but am trying to make the most out of an unfortunately regettable thing.