Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions .chloggen/opampextension-fix-self-status-reporting.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. receiver/filelog)
component: extension/opamp

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Fix opampextension self-reported status events (e.g. the fatal error emitted when the parent process disappears) being silently dropped when the component.Host does not implement componentstatus.Reporter"

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [49412]

# (Optional) One or more lines of additional information to render under the primary note.
subtext: |
Previously, opampAgent.Start wired its internal reportFunc to call
componentstatus.ReportStatus(host, event) directly. The service starts
extensions with the bare graph.Host, which does not implement
componentstatus.Reporter (only pipeline components are handed a
Reporter-capable host wrapper), so ReportStatus was a silent no-op and
self-reported status changes -- most notably the fatal error monitorPPID
emits when the collector's parent process disappears -- were discarded and
never reached the extension's status aggregator, leaving the collector
reporting healthy to the OpAMP server despite being orphaned.

reportFunc now routes self-reported events to the extension's own
ComponentStatusChanged handler, so they reach the status aggregator and the
orphaned collector is reported unhealthy.

change_logs: [user]
11 changes: 10 additions & 1 deletion extension/opampextension/opamp_agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type opampAgent struct {
resourceAttrs map[string]string

instanceUID uuid.UUID
extensionID component.ID

eclk sync.RWMutex
effectiveConfig *confmap.Conf
Expand Down Expand Up @@ -106,8 +107,15 @@ var (
)

func (o *opampAgent) Start(ctx context.Context, host component.Host) error {
selfInstanceID := componentstatus.NewInstanceID(o.extensionID, component.KindExtension)
o.reportFunc = func(event *componentstatus.Event) {
componentstatus.ReportStatus(host, event)
Comment thread
CodeBlackwell marked this conversation as resolved.
// The service starts extensions with the bare graph.Host, which does not
// implement componentstatus.Reporter (only pipeline components are handed
// a Reporter-capable host wrapper). componentstatus.ReportStatus would
// therefore silently no-op, so route self-reported events -- such as the
// fatal error monitorPPID emits when the parent process disappears --
// straight to this extension's own status aggregator.
o.ComponentStatusChanged(selfInstanceID, event)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Route fatal self-reports through the host reporter

With the normal collector service host, host implements componentstatus.Reporter; before this change the self-report went through that reporter, whose status notification path also sends StatusFatalError to the service AsyncErrorChannel. Calling the OpAMP watcher directly only updates this extension's internal aggregator, so when ppid is configured and the parent disappears the OpAMP health may flip but the collector no longer receives the fatal event and keeps running orphaned (and with reports_health: false this send has no initialized componentStatusCh to receive it). Please preserve the host reporter path when available and add the internal fallback for non-reporter hosts.

Useful? React with 👍 / 👎.

}

header := http.Header{}
Expand Down Expand Up @@ -332,6 +340,7 @@ func newOpampAgent(cfg *Config, set extension.Settings) (*opampAgent, error) {
serviceVersion: serviceVersion,
serviceInstanceID: serviceInstanceID,
instanceUID: uid,
extensionID: set.ID,
capabilities: cfg.Capabilities,
opampClient: opampClient,
resourceAttrs: resourceAttrs,
Expand Down
38 changes: 38 additions & 0 deletions extension/opampextension/opamp_agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,44 @@ func TestStartAvailableComponents(t *testing.T) {
assert.NoError(t, o.Shutdown(t.Context()))
}

// TestReportFuncReachesStatusAggregatorRegardlessOfHostReporter verifies that a
// status reported through o.reportFunc (e.g. the fatal error monitorPPID emits when
// the parent process disappears) always reaches the status aggregator, even when the
// component.Host passed to Start does not implement componentstatus.Reporter. Without
// this, PPID-orphan detection silently fails to mark the collector unhealthy.
func TestReportFuncReachesStatusAggregatorRegardlessOfHostReporter(t *testing.T) {
cfg := createDefaultConfig().(*Config)
set := extensiontest.NewNopSettings(extensiontest.NopType)

sa := &mockStatusAggregator{mtx: &sync.RWMutex{}}

o := newTestOpampAgent(cfg, set, &mockOpAMPClient{
setHealthFunc: func(_ *protobufs.ComponentHealth) error { return nil },
}, sa)

o.initHealthReporting()

// componenttest.NewNopHost() intentionally does not implement componentstatus.Reporter.
require.NoError(t, o.Start(t.Context(), componenttest.NewNopHost()))
require.NoError(t, o.Ready())

fatalErr := errors.New("collector was orphaned")
o.reportFunc(componentstatus.NewFatalErrorEvent(fatalErr))

require.Eventually(t, func() bool {
sa.mtx.RLock()
defer sa.mtx.RUnlock()
return len(sa.receivedEvents) == 1
}, 5*time.Second, 100*time.Millisecond)

sa.mtx.RLock()
defer sa.mtx.RUnlock()
require.Equal(t, componentstatus.StatusFatalError, sa.receivedEvents[0].event.Status())
require.Equal(t, fatalErr, sa.receivedEvents[0].event.Err())

require.NoError(t, o.Shutdown(t.Context()))
}

// availableComponentsHost mocks a receiver.ReceiverHost for test purposes.
type availableComponentsHost struct {
t *testing.T
Expand Down
Loading