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

Adding command requests_per_hour #34

Closed

Conversation

valleedelisle
Copy link
Contributor

Refactored the time calculation function to be more dynamic

@coveralls
Copy link

coveralls commented Nov 29, 2019

Coverage Status

Coverage increased (+0.07%) to 100.0% when pulling d95967f on valleedelisle:requests_per_hour into ad76cf9 on gforcada:master.

@valleedelisle valleedelisle force-pushed the requests_per_hour branch 2 times, most recently from ca13b48 to ba3eb5f Compare November 29, 2019 18:30
Refactored the time calculation function to be more dynamic
minutes=tm.minute % aggregate,
seconds=tm.second,
microseconds=tm.microsecond,
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valleedelisle would you mind explaining the logic of this adding and removing? 🤔 I get the seconds and microseconds removal, as to zero them, but the aggregate puzzles me a little bit.

I was playing with the request_per_minute.log file and I see that what counts as a request that belongs to a minute goes from the half a minute before to the half a minute after, i.e.

Requests logged starting from 12:12:30 until 12:13:29 will all belong to 12:13:0 minute. Is what you expected to do?

At least, we could mention that it is not bound to the exact minute/hour to avoid users getting confused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gforcada I based this part from this stackoverflow question. You're right when you say that :30 to :29 are going to be in the same minute. I didn't realize it would change the actual behavior maybe it would be better to keep the same behavior. The reason why I picked this is because we can pass any number of minutes to round up/down to.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actually it is fine, usually when I found solutions via stackoverflow I put a comment on top with the link to it, so one does not need lengthy explanations 👍

@gforcada
Copy link
Owner

gforcada commented Dec 1, 2019

@valleedelisle thanks for the pull request! 💯 I would like to get a comment on the logic of it, but otherwise it looks good! 👍

@gforcada
Copy link
Owner

gforcada commented Jan 6, 2020

@valleedelisle sorry I made a new release of haproxy_log_analysis with a complete rewrite of the commands and the main logic.

Sadly this means that this pull request can no longer be merged as it is 😞

On the up side, the commands now are more easier to write and perform much better 👍 I can try to recreate your additions here on a new pull request adapted to the new code base. Would it be ok with you? 🤔

@valleedelisle
Copy link
Contributor Author

@gforcada Nice! I'll look into this, thanks for maintaining this project. Honestly, I was able to achieve what I had to do and extract the stats I needed so I kind of lost interest into this for now. Feel free to take what I did and adapt it however you think it fits best.

Thanks again for the great job!

@gforcada gforcada mentioned this pull request Jan 6, 2020
@gforcada
Copy link
Owner

gforcada commented Jan 6, 2020

Done at #36

@gforcada gforcada closed this Jan 6, 2020
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.

None yet

3 participants