opentelemetry-sdk: sketch of an OpAMP integration#4646
opentelemetry-sdk: sketch of an OpAMP integration#4646xrmx wants to merge 11 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?
There was a problem hiding this comment.
I've renamed the entry point to pre_sdk_init_function so that we can add a post_sdk_init_function if required
pmcollins
left a comment
pmcollins
left a comment
There was a problem hiding this comment.
Thanks for doing this. Added a comment.
| # 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?
There was a problem hiding this comment.
Changed error handling in 830fd1b. If the user provided function raises any exception it'll break as any other loaded components doing that. Not sure we should catch more broadly, it's fine to crash at init time if something is wrong and for someone maybe a misconfigured sdk is worse than a sdk not working. But I don't have a strong opinion on that.
|
|
||
| # 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?
xrmx
force-pushed
the
opamp-prototype-integration
branch
from
23f3ff7 to
830fd1b
Compare
| # provided code as it's not strictly specified. We call OpAMP init before other code | ||
| # because people may want to have it blocking to get an updated config before setting | ||
| # up the rest of the SDK. | ||
| _init_opamp = _import_opamp() |
There was a problem hiding this comment.
My org is interested in using OpAMP first to report effective SDK configuration. For that use case, it would be convenient if there were also an OpAMP hook after _init_tracing, _init_metrics, and _init_logging, so the OpAMP client could inspect how providers, processors, exporters, resources, etc. were actually configured from the various sources (env vars, CLI flags, file config, defaults, and entry points).
With only a pre-init hook, the OpAMP implementation would have to re-create or re-invoke parts of the SDK configuration logic and infer the effective config, or it would have to poll the global providers and traverse the private object graphs after an arbitrary delay.
I can see the value of the current pre-init hook for influencing initial configuration before providers are wired, but a post-init hook seems useful for reporting effective config. This could be a separate PR.
Longer term, maybe it would be better to introduce a configuration controller/object that could be the source of truth for SDK config: it could resolve the config from the supported sources, provide the effective config for reporting, accept updates from OpAMP, and notify listeners when the configuration changes. What do you think?
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: