Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create HealthIndicatorService to simplify custom health indicators #2510

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

BrunnerLivio
Copy link
Member

@BrunnerLivio BrunnerLivio commented Jan 31, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Currently, a custom health indicator looks as follows:

@Injectable()
export class DogHealthIndicator extends HealthIndicator {
  constructor(private readonly dogService: DogService) {
    super();
  }

  async isHealthy(key: string): Promise<HealthIndicatorResult> {
    const dogs = await this.dogService.getDogs();
    const badboys = dogs.filter((dog) => dog.state === DogState.BAD_BOY);
    const isHealthy = badboys.length === 0;

    const result = this.getStatus(key, isHealthy, { badboys: badboys.length });

    if (isHealthy) {
      return result;
    }
    throw new HealthCheckError('Dog check failed', result);
  }
}

Terminus provides an API that falls short on the following points:

  • Uses inheritance over composition
  • Makes testing more difficult
  • The HealthIndicator.getStatus function has an undescriptive name and a difficult-to-read API interface

A new way to construct custom health indicator would be proposed in order to resolve these issues:

@Injectable()
export class DogHealthIndicator {
  constructor(
    private readonly dogService: DogService,
    private readonly healthIndicatorService: HealthIndicatorService,
  ) {}

  async isHealthy<const TKey extends string>(key: TKey) {
    const indicator = this.healthIndicatorService.check(key);

    const dogs = await this.dogService.getDogs();
    const badboys = dogs.filter((dog) => dog.state === DogState.BAD_BOY);
    const isHealthy = badboys.length === 0;

    if (!isHealthy) {
      return indicator.down({
        badboys: badboys.length,
      });
    }

    return indicator.up();
  }
}

What is the new behavior?

Simple, more readable code

Does this PR introduce a breaking change?

  • Yes
  • No => A deprecation warning will be logged, though the old API will still work as is

@BrunnerLivio BrunnerLivio changed the base branch from master to fix/mikro-orm-check-returns-always-healthy January 31, 2024 15:29
@delete-merged-branch delete-merged-branch bot deleted the branch beta January 31, 2024 15:30
@BrunnerLivio BrunnerLivio changed the base branch from fix/mikro-orm-check-returns-always-healthy to master January 31, 2024 21:24
@BrunnerLivio BrunnerLivio force-pushed the refactor/health-indicator-service branch 2 times, most recently from 0a150c6 to 07b2e75 Compare February 13, 2024 13:39
@BrunnerLivio BrunnerLivio force-pushed the refactor/health-indicator-service branch from 07b2e75 to 3573ede Compare January 23, 2025 13:34
@BrunnerLivio BrunnerLivio changed the base branch from master to beta January 23, 2025 18:53
@BrunnerLivio BrunnerLivio force-pushed the refactor/health-indicator-service branch 2 times, most recently from ddbccd0 to 1be876f Compare January 23, 2025 19:23
The abstract `HealthIndicator` class has been marked as deprecated as well as the `HealthCheckError` error.

Instead a `HealthIndicatorService` should be utilised
to indicate the state of a health indicator.
@BrunnerLivio BrunnerLivio force-pushed the refactor/health-indicator-service branch from 1be876f to 9f10a9b Compare January 23, 2025 19:33
@BrunnerLivio BrunnerLivio marked this pull request as ready for review January 23, 2025 19:37
@BrunnerLivio BrunnerLivio merged commit 4a830f1 into beta Jan 23, 2025
22 checks passed
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.

1 participant