Skip to content

refactor: Kill BaseObserver - migrate all observers to deps pattern#577

Merged
yairfalse merged 9 commits into
mainfrom
refactor/kill-base-observer
Jan 1, 2026
Merged

refactor: Kill BaseObserver - migrate all observers to deps pattern#577
yairfalse merged 9 commits into
mainfrom
refactor/kill-base-observer

Conversation

@yairfalse
Copy link
Copy Markdown
Collaborator

Summary

  • Migrate all 6 observers from *base.BaseObserver embedding to lean *base.Deps pattern
  • Delete BaseObserver struct and all its methods (~630 lines)
  • Delete Observer interface entirely (~178 lines)
  • Simplify telemetry to not require observer health checks

Changes by Observer

Observer Pattern Change
deployments NewDeploymentsObserver(name, config)New(config, deps)
scheduler NewSchedulerObserver(name, config)New(config, deps)
container Remove BaseObserver, add Run(ctx) with errgroup
node Remove BaseObserver, add Run(ctx)
container-api Remove BaseObserver, add Run(ctx)
container-runtime Remove BaseObserver, add Run(ctx)

New Pattern

type Observer struct {
    name   string
    deps   *base.Deps
    logger zerolog.Logger
    config Config
}

func New(config Config, deps *base.Deps) (*Observer, error)
func (o *Observer) Run(ctx context.Context) error  // blocks until cancelled

Metrics/Emitter Access

  • o.deps.Metrics.RecordEvent(o.name, eventType)
  • o.deps.Metrics.RecordError(o.name, eventType, errorType)
  • o.deps.Emitter.Emit(ctx, event)

Test plan

  • All internal/base tests pass
  • All observer tests pass
  • Full project builds
  • go vet and golangci-lint pass

🤖 Generated with Claude Code

yairfalse and others added 8 commits December 31, 2025 21:41
- Replace *base.BaseObserver embedding with deps *base.Deps
- Replace NewDeploymentsObserver() with New(config, deps)
- Replace Start()/Stop() with Run(ctx) that blocks until cancelled
- Update emitEvent to use deps.Metrics and deps.Emitter
- Update all tests to use new constructor and pattern

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace *base.BaseObserver embedding with explicit name, deps, logger
- Change constructor: NewSchedulerObserver(name, config) → New(config, deps)
- Change lifecycle: Start()/Stop() → Run(ctx) blocking until cancelled
- Update emitDomainEvent to use deps.Metrics and deps.Emitter
- Update events_watcher.go to use observer.logger field
- Update all test files for new constructor pattern

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace *base.BaseObserver embedding with explicit name, deps, logger
- Change constructor: NewContainerObserver(name, config) → New(config, deps)
- Change lifecycle: Start() → Run(ctx) with errgroup
- Replace log.Printf with zerolog logger
- Replace inherited RecordEvent/RecordError/RecordDrop with deps.Metrics
- Replace SendObserverEvent with deps.Emitter.Emit
- Fix pre-existing compliance issue: properly use bool return from cache lookup

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace *base.BaseObserver embedding with explicit name, deps, logger
- Remove Emitter from Config (now accessed via deps.Emitter)
- Change constructor: NewObserver(name, cfg) → New(cfg, deps)
- Change lifecycle: Start()/Stop() → Run(ctx) blocking until cancelled
- Replace Logger(ctx) with logger field
- Replace RecordEvent/RecordError with deps.Metrics calls
- Update all test cases for new constructor pattern

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace *base.BaseObserver embedding with explicit name, deps, logger
- Remove Emitter from Config (now accessed via deps.Emitter)
- Remove stopCh field (use context cancellation instead)
- Change constructor: NewAPIObserver(name, cfg) → New(cfg, deps)
- Change lifecycle: Start()/Stop() → Run(ctx) blocking until cancelled
- Update checkContainers to use deps.Metrics and deps.Emitter
- Update all test cases for new constructor pattern

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove *base.BaseObserver embedding
- Add explicit name, deps, logger fields
- Remove Emitter from Config (now via deps.Emitter)
- Combine Start/Stop/Run into single Run(ctx) with deferred cleanup
- Replace o.Logger(ctx) with o.logger field
- Replace o.RecordEvent/RecordError with deps.Metrics calls
- Replace o.emitter.Emit with deps.Emitter.Emit (nil-checked)
- Update OBSERVERS.md documentation examples

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Delete BaseObserver struct and all its methods (~250 lines removed)
- Delete observer_test.go (tested deleted code)
- Keep Observer interface for telemetry readiness checks
- Add mockObserver to telemetry_test.go for readiness tests

Observers now use lean deps pattern:
- Explicit name, deps, logger fields
- Run(ctx) instead of Start/Stop
- deps.Metrics.RecordEvent/RecordError
- deps.Emitter.Emit

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Delete observer.go (no longer needed)
- Remove observers parameter from InitTelemetry
- Simplify /ready endpoint to always return 200 (like /health)
- Delete readyHandler function (~35 lines)
- Simplify telemetry_test.go (~120 lines removed)

No production code was using the Observer interface.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors all 6 observers from an inheritance-based BaseObserver pattern to a lean dependency injection pattern using *base.Deps. The changes eliminate approximately 808 lines of code by removing the BaseObserver struct, Observer interface, and associated methods. The refactoring standardizes observer construction and execution while simplifying telemetry by removing observer health checks.

Key Changes:

  • Migrated all observers to use New(config, deps) constructor pattern instead of NewXxxObserver(name, config)
  • Replaced Start(ctx)/Stop() lifecycle with blocking Run(ctx) method
  • Switched from BaseObserver embedding to direct *base.Deps field for accessing metrics and emitter
  • Removed observer health check logic from telemetry endpoints (/ready now always returns 200 like /health)

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/observers/scheduler/observer.go Refactored to use deps pattern; Run() method blocks until context cancelled; metrics builder error handling changed
internal/observers/scheduler/observer_test.go Updated all tests to use new constructor signature and Run() lifecycle
internal/observers/scheduler/events_watcher*.go Updated tests to create observers using new pattern
internal/observers/node/observer.go Removed BaseObserver embedding; added Run() method with blocking behavior
internal/observers/node/observer_test.go Updated tests for new constructor and Run() pattern
internal/observers/deployments/observer.go Migrated to deps pattern; simplified Run() to directly run informer
internal/observers/deployments/observer_*_test.go Updated all test files to use new constructor and deps injection
internal/observers/container/observer.go Added errgroup for concurrent stage management in Run()
internal/observers/container-runtime/observer.go Consolidated Start/Run into single Run() method with inline cleanup
internal/observers/container-api/observer.go Removed stopCh channel in favor of context-based cancellation
internal/observers/container-api/observer_test.go Updated tests; removed health check and Start/Stop tests
internal/base/telemetry.go Removed observer parameter and health checking from InitTelemetry; /ready endpoint simplified
internal/base/telemetry_test.go Removed observer health check tests; simplified ready endpoint test
internal/base/observer.go Deleted entire file (BaseObserver and Observer interface)
internal/base/observer_test.go Deleted entire test file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/observers/scheduler/observer.go Outdated
Comment thread internal/observers/scheduler/observer.go
Comment thread internal/observers/deployments/observer.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Log warning when builder.Build() fails rather than silently ignoring
the error with nolint directive.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@yairfalse yairfalse merged commit a7d4589 into main Jan 1, 2026
1 of 3 checks passed
@yairfalse yairfalse deleted the refactor/kill-base-observer branch January 1, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants