[cmd/opampsupervisor] Receive and report effective config #31641
Conversation
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
evan-bradley
force-pushed
the
supervisor-effective-config
branch
2 times, most recently
from
9b31b09
to
7e60da3
Compare
| if message.EffectiveConfig != nil { | ||
| s.logger.Debug("Setting confmap") | ||
| fmt.Println(string(message.EffectiveConfig.ConfigMap.ConfigMap[""].Body)) | ||
| s.effectiveConfig.Store(string(message.EffectiveConfig.ConfigMap.ConfigMap[""].Body)) |
There was a problem hiding this comment.
Can we safely assume that the entire config is reported this way? Or should we iterate on the map and merge?
There was a problem hiding this comment.
This is a bit of an open question. I will open an issue in the spec repo to link to here and we can use that as a starting point for discussion.
There was a problem hiding this comment.
Spec issue: open-telemetry/opamp-spec#184
There was a problem hiding this comment.
Tested this a bit against BindPlane, it was able to correctly receive and report the config.
Only issue I ran into is that if the OpAMP server does not offer an a config with a key of "", the supervisor will panic. I think that can be addressed in a completely separate PR, though.
| traces: | ||
| receivers: [nop] | ||
| exporters: [nop] | ||
| extensions: [opamp] |
There was a problem hiding this comment.
It seems like this doesn't need the extensions portion here, the final config ends up with the opamp extension defined twice.
| if s.effectiveConfig.Load().(string) != newEffectiveConfig { | ||
| if s.mergedConfig.Load().(string) != newEffectiveConfig { | ||
| s.logger.Debug("Effective config changed.") | ||
| s.effectiveConfig.Store(newEffectiveConfig) | ||
| s.mergedConfig.Store(newEffectiveConfig) | ||
| configChanged = true | ||
| } |
There was a problem hiding this comment.
This seems like existing behavior, but is mergedConfig being modified concurrently?
If it is, shouldn't this Load and Store both happen atomically (e.g. using Swap)?
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
**Description:** <Describe what has changed.> Redacts primitive values in the effective config reported by the extension. Necessary for #31641 until open-telemetry/opentelemetry-collector#10139 is resolved. Relates to #32983
evan-bradley
force-pushed
the
supervisor-effective-config
branch
from
b8655b1
to
67d0e7f
Compare
|
Closing in favor of #33462. |
**Description:** Adds the ability to receive and run remote configurations from an OpAMP server, as well as to report the remote configuration status. This PR is just bringing #31641 up-to-date. **Link to tracking Issue:** Resolves #30622 **Testing:** <Describe what testing was performed and which tests were added.> Unit tests **Documentation:** <Describe the documentation added.> --------- Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com> Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Link to tracking Issue: Resolves #30622