beberlei / metrics

Simple library that abstracts different metrics collectors. I find this necessary to have a consistent and simple metrics (functional) API that doesn't cause vendor lock-in.
316 stars 38 forks source link

Why not flushing on destruct? #30

Closed ChristianRiesen closed 9 years ago

ChristianRiesen commented 9 years ago

I was just looking at the library and it struck me odd why there is no destructor defined that would flush. Is that on purpose and if so whats the rationale behind it? If there is nothing against it I'd gladly add it in.

lyrixx commented 9 years ago

Because it's better to be explicit. Then, right now, you have full control on when the flush is done. But you can still create an object that do that for you ;)

ChristianRiesen commented 9 years ago

The question is more then, what is the use case to actually collect data and then NOT flush? Sure, you can do it explicitly, but it should be so there is virtually no overhead on flushing if there is nothing to flush.

lyrixx commented 9 years ago

For example, if you use symfony2, all destructors will be called after the $response->send(). As you can see we call fastcgi_finish_request and so it becomes hard to debug your metric if something goes wrong.

Of course, we can still register a shutdown function that just call flush(). Like that, the end user could still be explicit.

So right now, I'm not :-1: nor :+1: for this issue.

Can we stay with this issue open for few days, in order to get more feedback ?

ChristianRiesen commented 9 years ago

If something goes horribly wrong at that stage, using metrics inside symfony is probably not the place that will reliably inform you about it either.

lyrixx commented 9 years ago

@nicolas-grekas: Do you have a thought on this one ?

nicolas-grekas commented 9 years ago

To me, magic is always a bad idea, and destructors are magic in PHP, so I'd say it's better not having auto-flush here (although that does not prevent anyone adding a wrapper that adds exactly this magic).

lyrixx commented 9 years ago

I agree with @nicolas-grekas