liip / LiipMonitorBundle

Integrates the LiipMonitor library into Symfony
http://liip.ch
467 stars 103 forks source link

Senseless setting of additional reporters, as in return always ArrayReport type #244

Closed feosys closed 4 years ago

feosys commented 4 years ago

What sense of setting additional/custom reporters, if in return always fixed ArrayReporter?

I have found it, when trying add custom reporter, which will also save additional data from \ZendDiagnostics\Result\AbstractResult::getData(). As ArrayReport does not do it.

https://github.com/liip/LiipMonitorBundle/blob/653b8578557385cf3fee4879d3bc3cb31d405020/Controller/HealthCheckController.php#L166

kbond commented 4 years ago

Sorry, I'm not too familiar with the controller or custom reporters but doesn't this line:

https://github.com/liip/LiipMonitorBundle/blob/653b8578557385cf3fee4879d3bc3cb31d405020/Controller/HealthCheckController.php#L177

Set the additional reporters passed?

feosys commented 4 years ago

yes, you are right. But what sense of that additional reporters, if in the end, in controller we return and use just default. https://github.com/liip/LiipMonitorBundle/blob/653b8578557385cf3fee4879d3bc3cb31d405020/Controller/HealthCheckController.php#L180

For examle: i need collect Result "data" (AbstractResult::getData()). Default ArrayReport doesn't collect it. How can i put my custom report implementation and return one more field with liip rest api?

kbond commented 4 years ago

Oh I see, you're looking to replace the array reporter with your own.

I think your best bet would be to extend the controller with your own implementation.

feosys commented 4 years ago

Yes, u are right. But anyway, what purpose of that additional reporters, if u could not use their collected data (in the context of that controller)?

пн, 17 авг. 2020 г., 22:04 Kevin Bond notifications@github.com:

Oh I see, you're looking to replace the array reporter with your own.

I think your best bet would be to extend the controller with your own implementation.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/liip/LiipMonitorBundle/issues/244#issuecomment-675084839, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7KWD5NGHGVUP3MB3ALJA3SBGEM3ANCNFSM4QB25DBQ .

kbond commented 4 years ago

Ok, looked back and it looks like I was the one to add this feature but forgot - oops! In my defense, it was 6 years ago - haha

Anyway, looks like it was added so the MailReporter could be used via the API... So not to change the output of the API, just to trigger some other reporter that does different things...

feosys commented 4 years ago

Aha, I see. Yeah, in the context of the Mail reporter it has sense. Thx a lot, now I can see full picture.

пн, 17 авг. 2020 г., 22:31 Kevin Bond notifications@github.com:

Ok, looked back and it looks like I was the one to add this feature - oops! In my defense it was 6 years ago - haha

Anyway, looks like it was added so the MailReporter could be used via the API... So not to change the output of the API, just to trigger some other reporter that does different things...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/liip/LiipMonitorBundle/issues/244#issuecomment-675098350, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7KWD4NB2RMMXF7W5HWJTDSBGHTDANCNFSM4QB25DBQ .