Update OpAMP client to have additional callback methods#4322
Update OpAMP client to have additional callback methods#4322kelseyma wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
| from opentelemetry._opamp.client import OpAMPClient | ||
|
|
||
|
|
||
| class Callbacks: |
There was a problem hiding this comment.
What do we think about making this an ABC (mostly to prevent it from being accidentally instantiated)?
There was a problem hiding this comment.
We can do that, I see that the go client makes them all optional
There was a problem hiding this comment.
I was looking into ABC and in order to keep all optional, it seemed like Callbacks() could still be instantiated. And if we wanted to fully prevent this, we'd need to mark at least one method (e.g. on_message) as abstract, but that would make it mandatory for subclasses.
Do we have a preferred behavior for the class? Go makes all callbacks optional, while the java client requires them. I think it could be reasonable to require on_message as that provides most of the value from using opamp client. Happy to align with whichever behavior we want to standardize on!
There was a problem hiding this comment.
This might be slightly overkill, but we can always do the following:
class Callbacks(abc.ABC):
def __new__(cls, *args, **kwargs):
if cls is Callbacks:
raise TypeError(f"Cannot instantiate abstract class '{cls.__name__}'")
return super().__new__(cls, *args, **kwargs)Even without this check, by extending abc.ABC most linters/IDEs should give a warning when attempting to directly instantiate Callbacks
| job.payload, | ||
| job.attempt, | ||
| job.max_retries, | ||
| job.max_retries + 1, |
There was a problem hiding this comment.
When testing, I noticed the logs said this:
WARNING - Job xyz failed attempt 1/1
WARNING - Job xyz failed attempt 2/1
max_retries is 1, meaning we are doing 2 attempts total (normal attempt + 1 retry). I added plus 1 so the logs now read:
WARNING - Job xyz failed attempt 1/2
WARNING - Job xyz failed attempt 2/2
|
|
||
|
|
||
| def test_dispatch_order(): | ||
| """Verify the opamp-go dispatch order: on_connect -> on_message -> on_error.""" |
| logger.error( | ||
| "Job %r dropped after max retries", job.payload | ||
| ) | ||
| logger.exception(exc) |
There was a problem hiding this comment.
Why removing this? The on_connect_failed callback does not have the job and doesn't know if we are giving up
There was a problem hiding this comment.
The exception (minus the traceback) is logged earlier in the failed attempt msg, and we’re still logging when we’re giving up. During testing for on_connect_failed, I was getting a 100+ lines traceback in the logs each connection attempt which made it a bit noisy. Happy to add it back if we’d prefer to keep the traceback
| logger.warning( | ||
| "Job %r handler failed with: %s", job.payload, exc | ||
| _safe_invoke( | ||
| self._callbacks.on_message, self, self._client, message |
There was a problem hiding this comment.
Do we want to pass the entire message here? Looks like the go implementation passes a subset of the message fields to on_message, and does not include error_response. Would also remove ambiguity about handling errors in on_message rather than on_error.
4 participants
Description
Update the OpAMP client to use a
Callbacksclass, bringing Python closer to the existing opamp-go and Java opamp-client implementations. Adding a minimal set of callbacks at the moment (on_connect,on_connect_failed,on_error,on_message) with no-op default methods so new callbacks can be added in the future without breaking existing subclasses.Hoping to work on client-side sending of messages after state changes next, which might add some additional callbacks and also allow us to not have to pass
OpAMPAgentinto the callbacks.Also, removed some logging as I found it to be a little verbose while testing (e.g. 100 line traceback logged when can't connect to server) and exception was logged in multiple places. Happy to add back if preferred to keep.
Note: this is a breaking change to the OpAMPAgent constructor (
message_handleris replaced byCallbacks), but the package hasn't been released yet. Some prior discussion on this: #3635 (comment)Type of change
How Has This Been Tested?
Tested with a local sample app (implemented callbacks subclass) and test server
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.