Add queuing time metric

[Tomasz-Kluczkowski]

Hi Dani,

As promised I am making a PR to add more metrics (this time just task queuing time because I had to refactor a lot).

The reason for a refactor was I noticed I am creating a god object :) inside of the Exporter so I moved the logic that is not really its concern out of it (event handlers). This allows me to create custom handlers easily and reuse base class's logic for the standard counter behaviours.

I also added some stuff in Readme.md on contributing from what I had to do to get the project going.

Also I tried to add argument and return types as I went through the code - it really helps reading it and getting nice intellisense in Pycharm :).

Added unit tests for each event handler type checking all possible execution paths.

I know it's a lot of change but it makes the project much more modular and easier to work with so hopefully you like it too :).

Looking forward to your opinions.

I am now working on adding latency time (difference in time between task received - task sent and queue lengths but those are a bit trickier as require that the user enables send_task_sent_events I think and for queue length - each broker does it differently if at all...)

Tom

Queuing time in action

[danihodovic]

First of all, I appreciate the time you put into this pull request.

You have combined a major refactoring with the addition of one metric. I think refactorings and feature additions should be kept separate because it makes it easier to review. In a refactoring we can discuss architecture and taste, while in a feature addition we're interested in if the code works.

This is effectively a rewrite of the entire codebase. I like to write code that is concise and easy to read. These changes go in the opposite direction. There is a lot of OOP boilerplate that I think is redundant. You have added ~800 lines of code when the entire exporter logic fits in ~150.

Programmers are fickle, what some consider "maintainable" and good taste, others consider a mess.

I won't merge the pull request as it stands right now. I have limited time to maintain this exporter and I need to make sure it works for my clients. Adding 800 lines of code that I don't understand for a single metric isn't a good tradeoff in my opinion.

I'll happily merge a pull request that adds the queuing logic without rewriting the entire exporter. We can discuss architectural changes in a separate pull-request, but unless it's concise and easy to understand (again, subjective) I'm unlikely to merge it.

I hope you won't take it personal. If I rewrote a project you maintained in my taste it would be equally rejected :)

Miłego weekendu :beers:

[Tomasz-Kluczkowski]

Hey no personal here at all Dani :)

I thought you will not like this but got carried away (and had lots of fun during it - it all for the learning).

Anyway I have the initial branch still which changes like 3 lines to add this metric so I shall push that and we can forget about the refactoring.

I’ll try to push it during weekend.

Thx for looking into it.

Tom

On 16 Apr 2021, at 17:47, Dani Hodovic @.***> wrote:



First of all, I appreciate the time you put into this pull request.

You have combined a major refactoring with the addition of one metric. I think refactorings and feature additions should be kept separate because it makes it easier to review. In a refactoring we can discuss architecture and taste, while in a feature addition we're interested in if the code works.

This is effectively a rewrite of the entire codebase. I like to write code that is concise and easy to read. These changes go in the opposite direction. There is a lot of OOP boilerplate that I think is redundant. You have added ~800 lines of code when the entire exporter logic fits in ~150.

Programmers are fickle, what some consider "maintainable" and good taste, others consider a mess.

I won't merge the pull request as it stands right now. I have limited time to maintain this exporter and I need to make sure it works for my clients. Adding 800 lines of code that I don't understand for a single metric isn't a good tradeoff in my opinion.

I'll happily merge a pull request that adds the queuing logic without rewriting the entire exporter. We can discuss architectural changes in a separate pull-request, but unless it's concise and easy to understand (again, subjective) I'm unlikely to merge it.

I hope you won't take it personal. If I rewrote a project you maintained in my taste it would be equally rejected :)

Miłego weekendu 🍻

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/danihodovic/celery-exporter/pull/31#issuecomment-821304486, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGGVI2IA4FINRAYNGW3IOOTTJBSYDANCNFSM4Z43F2JQ.