-
Notifications
You must be signed in to change notification settings - Fork 784
Add a basic http OpAMP client #3635
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
base: main
Are you sure you want to change the base?
Conversation
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/agent.py
Outdated
Show resolved
Hide resolved
@open-telemetry/opamp-spec-approvers could you help review this? thanks! |
3e67115
to
e5fced7
Compare
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/transport/requests.py
Show resolved
Hide resolved
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/client.py
Outdated
Show resolved
Hide resolved
1d2061f
to
04d8923
Compare
4cfbf33
to
a746292
Compare
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/version.py
Outdated
Show resolved
Hide resolved
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/client.py
Outdated
Show resolved
Hide resolved
1d85953
to
cf479f2
Compare
I can't unresolve threads, but I've replied above. |
Thank you for working on this.
One thing that would be great to add is interoperability tests between Go and Python implementations. Ideally we would have both pairs (Client in Python, Server in Go and the vice versa) connecting and performing OpAMP exchanges to make sure the pair works correctly together. OpAMP Go implementation presumably is the most complete implementation at the moment, so all other languages (including Python) could use it as a reference implementation to test against. I think we can spend a bit time adding any necessary tooling to opamp-go that makes this possible (mock servers, clients that probe capabilities, etc). Unfortunately I don't have time myself but if anyone wants to work on the design and implementation I can dedicate some time to review the design. |
This PR has some recorded e2e tests doing this, we don't test many scenarios but at least we test against a real response.
That would be helpful indeed |
Still not building content
Drop opentelemetry api fixed version from requirements
And a custom session to RequestsTransport
…s already private
b48e242
to
3eb1769
Compare
Missed .pyi exclusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving this an approving "python review" but I don't know much about OpAmp. Looks good enough to merge though as an initial prototype with proper disclaimers.
🚢
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/agent.py
Outdated
Show resolved
Hide resolved
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/agent.py
Outdated
Show resolved
Hide resolved
opamp/opentelemetry-opamp-client/src/opentelemetry/_opamp/agent.py
Outdated
Show resolved
Hide resolved
agent.send(payload=message) | ||
|
||
opamp_client = OpAMPClient( | ||
endpoint="http://localhost:4320/v1/opamp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to include some instructions on how to regenerate the VCR fixtures.
Alternatively, would it be possible to run the real server during the actual tests to avoid having to maintain fixtures? If it's easy enough and not flaky, it would be better IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some instructions but they are using an Elastic specific collector sending data to Elastic cloud: https://github.com/elastic/elastic-otel-python/blob/main/tests/opamp/README.md
If it's fine I can copy them otherwise I can add a TODO to recreate them with just the opamp-go server implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah i see, there's no pure collector-contrib OpAmp server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an opamp-go example server but it doesn't offer a way set a config file for the agents without touching the code. Filed this open-telemetry/opamp-go#456
Description
This introduces a basic OpAMP http client for handling remote configuration. The client implements a bunch of capabilities (ReportsStatus, ReportsHeartbeat, AcceptsRemoteConfig, ReportsRemoteConfig) that are enough to get a remote config from an opamp server, parse it, apply it and ack it. Since OTel / OpAMP do not standardize APIs, config options or environment variables the distros are required to provide code doing so.
OTel Python distros would need to provide their own message handler callback that implements the actual change of whatever configuration their backends sends.
In practice distro would need to do something like the following:
The module is called _opamp because it's a bit early to standardize on an api. The code is divided roughly in:
OpAMP reference: https://opentelemetry.io/docs/specs/opamp/.
This is tested against https://github.com/elastic/opentelemetry-collector-components/ that is using the opamp-go implementation.
TODO:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.