opentelemetry-sdk: sketch of an OpAMP integration#4646
opentelemetry-sdk: sketch of an OpAMP integration#4646xrmx wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
xrmx
force-pushed
the
opamp-prototype-integration
branch
from
f2f49fd to
579da8c
Compare
|
|
||
| # OpAMP is a system created to configure OpenTelemetry SDKs with a remote config. | ||
| # This is different than other init helpers because setting up OpAMP requires distro | ||
| # provided code as it's not strictly specified. We call OpAMP init before other code |
There was a problem hiding this comment.
In my distro I initialize the OpAMP client after the sdk has been setup but can't exclude other scenarios
There was a problem hiding this comment.
Should we add this a SIG topic to get the execution order?
pmcollins
left a comment
pmcollins
left a comment
There was a problem hiding this comment.
Thanks for doing this. Added a comment.
| # the resource and instantiate an OpAMP agent. | ||
| # Since configuration is not specified every implementors may have its own. | ||
| # Refer to opentelemetry-opamp-client package on how to setup the OpAMP agent. | ||
| _, opamp_init_func = _import_config_components( |
There was a problem hiding this comment.
Would it be better to read and handle the entry point directly here rather than use _import_config_components? (it throws an exception if there's no match).
| # up the rest of the SDK. | ||
| try: | ||
| _init_opamp = _import_opamp() | ||
| _init_opamp(resource=resource) |
There was a problem hiding this comment.
Is it possible for _init_opamp to raise a RuntimeError? If so, it could be slightly confusing as the logs would say no init function found instead of the actual error.
Also, is it possible for it to throw other types of exceptions? Would we want to catch more broadly to ensure we don't prevent the rest from initializing?
| raise RuntimeError(f"{id_generator_name} is not an IdGenerator") | ||
|
|
||
|
|
||
| def _import_opamp() -> callable[[...], None]: |
There was a problem hiding this comment.
callable is a builtin function, not a valid type, right?
I think this should be Callable[..., None] using collections.abc. You'll need to add it to the imports.
| def _import_opamp() -> callable[[...], None]: | |
| def _import_opamp() -> Callable[..., None]: |
|
|
||
| # OpAMP is a system created to configure OpenTelemetry SDKs with a remote config. | ||
| # This is different than other init helpers because setting up OpAMP requires distro | ||
| # provided code as it's not strictly specified. We call OpAMP init before other code |
There was a problem hiding this comment.
Should we add this a SIG topic to get the execution order?
4 participants
Description
This is a basic integration for setting up OpAMP in the sdk configurator.
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 Contrib Repo Change?
Checklist: