-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
♻️ Refactor Create Queue Health Indicator #4513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top quality work. I am into what you went for.
🌟
implements IHealthIndicator | ||
{ | ||
constructor( | ||
private queueBaseService: QueueBaseService, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private queueBaseService: QueueBaseService, | |
private queueService: QueueBaseService, |
Just because we are passing other services that satisfy the QueueBaseService interface.
private indicatorKey: string, | ||
private serviceName: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's think if some enums limiting the values that can be passed here would be something that would benefit us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now sure I follow, could you elaborate on what you mean here or provide an example? :)
async isHealthy(): Promise<HealthIndicatorResult> { | ||
return await this.handleHealthCheck(); | ||
} | ||
|
||
async isActive(): Promise<HealthIndicatorResult> { | ||
return await this.handleHealthCheck(true); | ||
} | ||
|
||
async handleHealthCheck(checkActive = false) { | ||
const isReady = this.queueBaseService.isReady(); | ||
|
||
if (!isReady) { | ||
Logger.verbose(`${this.serviceName} is not ready`, this.logContext); | ||
|
||
throw new HealthCheckError( | ||
`${this.serviceName} Health`, | ||
this.getStatus(this.indicatorKey, false) | ||
); | ||
} | ||
|
||
if (!checkActive) { | ||
Logger.verbose(`${this.serviceName} is ready`, this.logContext); | ||
|
||
return this.getStatus(this.indicatorKey, true); | ||
} | ||
|
||
const isPaused = await this.queueBaseService.isPaused(); | ||
const isActive = isReady && !isPaused; | ||
|
||
if (!isActive) { | ||
Logger.verbose(`${this.serviceName} is not active`, this.logContext); | ||
|
||
throw new HealthCheckError( | ||
`${this.serviceName} Health`, | ||
this.getStatus(this.indicatorKey, false) | ||
); | ||
} | ||
|
||
Logger.verbose(`${this.serviceName} is active`, this.logContext); | ||
|
||
return this.getStatus(this.indicatorKey, true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can avoid the use of argument flags and with a single function compose both methods in a more maintainable way. What do you think?
async isHealthy(): Promise<HealthIndicatorResult> { | |
return await this.handleHealthCheck(); | |
} | |
async isActive(): Promise<HealthIndicatorResult> { | |
return await this.handleHealthCheck(true); | |
} | |
async handleHealthCheck(checkActive = false) { | |
const isReady = this.queueBaseService.isReady(); | |
if (!isReady) { | |
Logger.verbose(`${this.serviceName} is not ready`, this.logContext); | |
throw new HealthCheckError( | |
`${this.serviceName} Health`, | |
this.getStatus(this.indicatorKey, false) | |
); | |
} | |
if (!checkActive) { | |
Logger.verbose(`${this.serviceName} is ready`, this.logContext); | |
return this.getStatus(this.indicatorKey, true); | |
} | |
const isPaused = await this.queueBaseService.isPaused(); | |
const isActive = isReady && !isPaused; | |
if (!isActive) { | |
Logger.verbose(`${this.serviceName} is not active`, this.logContext); | |
throw new HealthCheckError( | |
`${this.serviceName} Health`, | |
this.getStatus(this.indicatorKey, false) | |
); | |
} | |
Logger.verbose(`${this.serviceName} is active`, this.logContext); | |
return this.getStatus(this.indicatorKey, true); | |
} | |
async isHealthy(): Promise<HealthIndicatorResult> { | |
const isReady = this.checkIsReady(); | |
Logger.verbose(`${this.serviceName} is ready`, this.logContext); | |
return this.getStatus(this.indicatorKey, true); | |
} | |
async isActive(): Promise<HealthIndicatorResult> { | |
const isReady = this.checkIsReady(); | |
const isPaused = await this.queueBaseService.isPaused(); | |
const isActive = isReady && !isPaused; | |
if (!isActive) { | |
Logger.verbose(`${this.serviceName} is not active`, this.logContext); | |
throw new HealthCheckError( | |
`${this.serviceName} Health`, | |
this.getStatus(this.indicatorKey, false) | |
); | |
} | |
Logger.verbose(`${this.serviceName} is active`, this.logContext); | |
return this.getStatus(this.indicatorKey, true); | |
} | |
private checkIsReady(): boolean { | |
const isReady = this.queueBaseService.isReady(); | |
if (!isReady) { | |
Logger.verbose(`${this.serviceName} is not ready`, this.logContext); | |
throw new HealthCheckError( | |
`${this.serviceName} Health`, | |
this.getStatus(this.indicatorKey, false) | |
); | |
} | |
return isReady; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that one function will check isHealth and isActive altogether, and return one united status?
What change does this PR introduce?
Create and use queue health indicator base abstract class.
Why was this change needed?
In order to extract common code to base class so we could reuse it.
Other information (Screenshots)