Skip to content
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

OnOpampConnectionSettings/Accepted callbacks need re-thinking #261

Open
tigrannajaryan opened this issue Mar 5, 2024 · 4 comments
Open
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@tigrannajaryan
Copy link
Member

The comments say that the Client implementation (the caller) will attempt to re-connect using new settings.

The example implementation works completely differently, the callback tries the re-connection.

Which is the right way?

To summarize this is what the example is supposed to do:

  1. OnOpampConnectionSettings callback called when new settings are offered by the Server.
  2. The callback implementation first pre-verifies the settings (e.g. check certificates, etc).
  3. If checks in (2) pass the callback implementation logs that it is starting to reconnect, creates a reconnection goroutine and returns from the callback.
  4. Reconnection goroutine stops the Client, creates a new Client with new connection settings and wait for successful OnConnect() callback.
  5. If OnConnect() is not called within a predefined period of time reconnection goroutine assumes the new connection settings are bad, reverts to the old connection settings and re-creates the Client again.
  6. We get rid of OnOpampConnectionSettingsAccepted(), it is not needed.

The example implementation of steps 4 and 5 is not complete today (e.g not waiting for OnConnect), but can be modified to match the above steps.

See also comment thread here: open-telemetry/opentelemetry-collector-contrib#30237 (comment)

@tigrannajaryan
Copy link
Member Author

@srikanthccv
Copy link
Member

I listened to the meeting recording, and this sounds good to me. However, making it non-blocking leaves us with one question: what would be the mechanism for the client to report back to the server whether it accepted or rejected the connection settings offer?

@tigrannajaryan
Copy link
Member Author

what would be the mechanism for the client to report back to the server whether it accepted or rejected the connection settings offer?

It can be done using a health message. We don't have any other way to report errors from the agent currently.

tigrannajaryan added a commit to tigrannajaryan/opamp-go that referenced this issue Mar 25, 2024
Contributes to open-telemetry#261

I intentionally left undefined the blocking vs nonblocking
requirement for the callback. I suggest that we refine this
after we make implementations and settle on a particular
behavior.

A continuation of this PR is needed that implements steps 4 and 5
of the flow described here:
open-telemetry#261
andykellr pushed a commit that referenced this issue Mar 25, 2024
Contributes to #261

I intentionally left undefined the blocking vs nonblocking
requirement for the callback. I suggest that we refine this
after we make implementations and settle on a particular
behavior.

A continuation of this PR is needed that implements steps 4 and 5
of the flow described here:
#261
@tigrannajaryan
Copy link
Member Author

Steps 1-3 and 6 done in #266

Steps 4-5 remaining.

@tigrannajaryan tigrannajaryan added help wanted Extra attention is needed enhancement New feature or request and removed bug Something isn't working documentation Improvements or additions to documentation labels Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants