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

Add API Requests By User #148

Closed
wants to merge 3 commits into from
Closed

Conversation

dfarr
Copy link
Contributor

@dfarr dfarr commented Apr 5, 2018

This adds a report to collect the top 20 API users each day, and
log the total number of requests each hour. Reports this info in
a simple table, this is handy for identifying users who are
approaching, or over the API rate limit.

An example of the API Rate Limit page:
screen shot 2018-04-05 at 3 38 40 pm

This adds a report to collect the top 20 API users each day, and
log the total number of requests each hour. Reports this info in
a simple table, this is handy for identifying users who are
approaching, or over the API rate limit.
@larsxschneider
Copy link
Collaborator

👋 @dfarr ! I have a few questions:

(1) Was there a specific reason to print the numbers per hour? I am asking because the finest granularity for all other numbers in Hubble is a "day" and I wonder if we should stick to that.

(2) Would it make sense to move your "Top API Users" into the API requests pages?

Thanks for this PR, this looks really useful 👍 😊

@dfarr
Copy link
Contributor Author

dfarr commented Apr 10, 2018

I think it would make sense to do top API users - does regenerating this every day make sense? If so it would be a pretty simple tweak to the script and simply moving the table to a different page :)

@toddocon
Copy link
Contributor

toddocon commented Apr 10, 2018

I think generating this report every day makes sense. I have some very heavy API (and polling) users and would like to make this information more visible.

Question, does this report still generate if rate limiting is not enabled? I assume this only makes sense if rate limiting is on. There are other logs and methods to pull out the heavy polling users. That's what I use now.

@larsxschneider
Copy link
Collaborator

Yeah, top API users per day makes total sense. @dfarr Do you want to tackle this?

@toddocon AFAIK this should work with and without rate limiting enabled.

@dfarr
Copy link
Contributor Author

dfarr commented Apr 10, 2018

Will do 👍

From my understanding, the api-requests-by-user.sh script will extract the data regardless of whether or not rate limiting is enabled. Rate limiting information does surface in some fields of the unicorn logs, but these fields are ignored by this script.

dfarr and others added 2 commits April 16, 2018 13:51
Modify the report to tabulate the top API useres each day, display
this information in housekeeping/api-requests
@dfarr
Copy link
Contributor Author

dfarr commented Apr 16, 2018

Updated! Here is a screenshot of the updated housekeeping/api-requests page:

screen shot 2018-04-16 at 1 38 59 pm

@toddocon
Copy link
Contributor

I like it! Can't wait to get this in my version.

Copy link
Contributor

@pluehne pluehne left a comment

Choose a reason for hiding this comment

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

@dfarr: Thank you very much. That’s a very nice addition to Hubble, and your code is well-written! I just have two minor remarks below that you could address 🙂.

@@ -0,0 +1,13 @@
from .Report import *


Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a short description of this report as a comment, as we have with the other reports?

@@ -50,6 +50,20 @@ <h3>API Requests</h3>
</div>
</div>

<div class="chart-placeholder">
<h3>Top API Requests by Users</h3>
Copy link
Contributor

@pluehne pluehne Apr 23, 2018

Choose a reason for hiding this comment

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

I’d drop the “Top” in the title, because it’s common for our charts only to show the top entries (for instance, in the Git traffic table).

@larsxschneider
Copy link
Collaborator

@dfarr Thank you very much 😃 👍

I rebased your PR and made a few minor modifications. Please take a look: f14daab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants