Extending recommendation for agent_disconnect to plain HTTP transport

[Rsingh2805]

From the spec, the current recommendation is

https://github.com/open-telemetry/opamp-spec/blob/main/specification.md#plain-http-transport-1

Plain HTTP Transport

The Client is considered logically disconnected as soon as the OpAMP HTTP response is completed. It is not necessary for the Client to send AgentToServer message with agent_disconnect field set since it is always implied anyway that the Client connection is gone after the HTTP response is completed.
HTTP keep-alive may be used by the Client and the Server but it has no effect on the logical operation of the OpAMP protocol.
The Server may use its own business logic to decide what it considers an active Agent (e.g. an Client that continuously polls) vs an inactive Agent (e.g. a Client that has not made an HTTP request for a specific period of time). This business logic is outside the scope of OpAMP specification.

However, if a plain HTTP opamp connection restarts, we've seen customers(in VM based environments) complain about seeing 2 connections until the server cleanup mechanism kicks in.

A solution to this could be to just have the recommendation for Plain HTTP OpAMP clients to send the agent_disconnect flag wherever possible. If not possible, then the server can still have a cleanup mechanism in place for things like kill -9 and similar sorts.

[tigrannajaryan]

I agree sending "disconnect" can be useful even for plain http. This is especially useful for large reporting intervals. The server typically needs to wait for missed heartbeats for 2 or more reporting intervals before declaring the agent "gone" and if you choose a few minute interval that ends up being quite some time before the agent is marked as gone.

Adding this to the spec as "SHOULD" and implementing in opamp-go is not very hard. We probably need to be careful in the implementation so that delivering disconnect message doesn't block shutdown of the agent for longer than we want, but that should be doable (and anyway that same concern applies to WebSockets).

@andykellr @evan-bradley @michel-laterman what do you think?

[michel-laterman]

Just so I understand, are we going to change the spec to recommend sending disconnect on all messages?
Or will it try to mimic websocket behaviour and send disconnect when the agent shuts down or receives new OpAMP connection settings, as our current state diagrams indicate?

[tigrannajaryan]

Just so I understand, are we going to change the spec to recommend sending disconnect on all messages?

No, not on all messages.

Or will it try to mimic websocket behaviour and send disconnect when the agent shuts down or receives new OpAMP connection settings, as our current state diagrams indicate?

Yes. When the agent shuts down it will include "agent_disconnect" field in the last AgentToServer message.

[michel-laterman]

The shutdown case sounds good; do we also want to send it when the client will attempt to use new OpAMP connection settings (as websocket clients currently do)??

[tigrannajaryan]

The shutdown case sounds good; do we also want to send it when the client will attempt to use new OpAMP connection settings (as websocket clients currently do)??

Yes, I think it should.

[michel-laterman]

I think it's a good idea to make disconnect signalling consistent behaviour between the transports

[Krishnachaitanyakc]

@michel-laterman I've opened PR #353 to address this. The change updates the Plain HTTP Transport section under "Closing Connection" to SHOULD-recommend agent_disconnect on shutdown and when switching connection settings, keeping the server-side cleanup as a fallback for ungraceful terminations.