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 Billing Graphs & Data to API #8245

Merged
merged 9 commits into from
Feb 20, 2018
Merged

Conversation

richardlawley
Copy link
Contributor

This PR makes billing graphs available through the API, along with the data backing them for use generating graphs in an external system. Updated documentation demonstrates the new API functions available.

As part of this, the billing_graph and bandwidth_graph pages were moved into the standard graph structure. The remaining billing_graph.php and bandwidth_graph.php pages have been converted to redirect to the new location, but this should really only affect people who have been accessing them directly, or that a page opened prior to updating librenms might try loading the old pages. If preferred, we can simply remove these pages.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@kkrumm1 kkrumm1 added the API label Feb 12, 2018
@richardlawley
Copy link
Contributor Author

The 5 scrut issues are false positives - either it has detected the wrong type signature for the 3rd party JpGraph library, or it's detecting a duplicate local function in different pages (only one will ever get included).

@laf
Copy link
Member

laf commented Feb 12, 2018

Let us know when you are done pushing updates so we can code review :)

@richardlawley
Copy link
Contributor Author

I'm done. Was fixing scrut issues then found a bug when testing on my live setup.

@laf
Copy link
Member

laf commented Feb 15, 2018

I'm getting this for Transfer graphs within billing (confirmed it works ok in current master):

image

@richardlawley
Copy link
Contributor Author

Hmm, that's tricky to diagnose. Can you let me know the URL of one of the images? I've got this PR applied to our live setup and everything is working fine for me

Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

Awesome work as always :)

Some comments left inline.

curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/graphs/bits
curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/graphs/bits?from=1517443200
curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/graphs/bits?from=1517443200&to=1517788800
curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/graphs/monthly
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth documenting this returns a png no matter whether you say you want svg graphs or not.

Example:

```curl
curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/graphdata/bits
Copy link
Member

Choose a reason for hiding this comment

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

This isn't valid:

{
    "status": "error",
    "message": "from parameter must be supplied"
}

Copy link
Member

Choose a reason for hiding this comment

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

@richardlawley This now returns a http 500.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unable to reproduce this again, on both my dev and live installs. Do you get a log message, or perhaps when adding &debug to the URL?

Copy link
Member

Choose a reason for hiding this comment

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

Your latest commit fixes that now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just found that the bills/1/history/1/graphdata/bits was giving a 500 error - is it possible you meant this route rather than bills/1/graphdata/bits ?

If so, this is now fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Possibly yes but it's definitely fixed as I checked out your previous commit and it broke again.

doc/API/Bills.md Outdated

Retrieve the data used to draw a graph so it can be rendered in an external system

Route: `/api/v0/bills/:id/graphdata/:graph_type
Copy link
Member

Choose a reason for hiding this comment

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

Should this be called /api/v0/bills/:id/data/:type

Also missing the backtick on the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better as graphdata, because the data is pre-processed for use on a graph (number of data points is reduced). The graph code was actually refactored so that this API call, the graph API calls and the graphs themselves all call the same functions to get data.

Copy link
Member

Choose a reason for hiding this comment

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

Ok no worries.

Retrieve the data for a graph of a previous period of a bill, to be rendered in an external system

Route: `/api/v0/bills/:id/history/:bill_hist_id/graphdata/:graph_type`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe: /api/v0/bills/:id/history/:bill_hist_id/data/:type

curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/history/1/graphdata/bits
curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/history/1/graphdata/hour
curl -H 'X-Auth-Token: YOURAPITOKENHERE' https://librenms.org/api/v0/bills/1/history/1/graphdata/day
Copy link
Member

Choose a reason for hiding this comment

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

The output for /day:

            "17\nJan",
            "18\nJan",
            "19\nJan",
            "20\nJan",

Should we add \n here as that should be potentially dealt with on the client end. Instead do 17 Jan or 17-Jan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just trying to work out the best way to handle this - it's applicable to all of the transfer graphs. Wondering whether to just return the unix timestamps of the start/end of each item and let all formatting be done on the client end. Only potential problem is if it introduces a timezone dependency

@laf
Copy link
Member

laf commented Feb 15, 2018

Hmm, that's tricky to diagnose. Can you let me know the URL of one of the images? I've got this PR applied to our live setup and everything is working fine for me

Sure. It's just /bill/bill_id=4/view=transfer/

@laf
Copy link
Member

laf commented Feb 15, 2018

Graph image:

https://web01.lathwood.uk/graph.php?type=bill_historictransfer&id=4&from=1516060800&to=1518739199&imgtype=day&width=1190&height=250frequency

Working one:

https://web01.lathwood.uk/bandwidth-graph.php?bill_id=4&bill_code=&from=1516060800&to=1518739199&type=day&imgbill=1&x=1190&y=250frequency

I have no idea where frequency is coming from in that!

@laf laf closed this Feb 15, 2018
@laf laf reopened this Feb 15, 2018
@laf
Copy link
Member

laf commented Feb 15, 2018

Sorry hit the wrong button

@laf
Copy link
Member

laf commented Feb 15, 2018

Found it:

https://p.libren.ms/view/d6c5f89b

@richardlawley
Copy link
Contributor Author

Not quite sure what it is you're showing with that link - lines 13/22 in your diff don't seem to correspond with the same file from the PR. Is it possible there's another change you've got merged in causing a conflict? Comparing to https://github.com/librenms/librenms/pull/8245/files#diff-b96e6cb436083334eca02bb816d9c017 I don't have any lines which just contain $bi .= '>"; - they are $bi .= '$type'>";.

I presume the images work fine if you remove frequency from the URL?

@laf
Copy link
Member

laf commented Feb 15, 2018

I don't have any lines which just contain $bi .= '>"; - they are $bi .= '$type'>";.

I presume the images work fine if you remove frequency from the URL?

Correct. And it's the $type that's causing the issue.

@richardlawley
Copy link
Contributor Author

Ok, all sorted:

  • $type parameter removed from transfer graph images (wasn't being used anyway)
  • API Documentation updated to mention PNG
  • Ticklabels format for monthly/days graph return YYYY-mm-dd or YYYY-mm-dd HH:mm:ss. Graphs updated to parse this and display as before
  • reducefactor API parameter documented
  • reducefactor automatically calculated if not supplied (default is to aim for ~400 samples).
  • Minor transfer graph fixes - hourly graph should start from 00:00 instead of first sample, and monthly graph displays as MMM yyyy for calendar month billing (rather then dd MMM yyyy - dd MMM yyyy)
  • Added max/average/last stats to bits graph so client can generate rrdtool-style max/ave/current display

Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

Just the last comment that needs addressing and this looks good.

@scrutinizer-notifier
Copy link

The inspection completed: 6 new issues, 11 updated code elements

@laf laf merged commit 1082989 into librenms:master Feb 20, 2018
@laf
Copy link
Member

laf commented Feb 20, 2018

Big thanks as always for adding this information in @richardlawley

@Harpreetsinghbedi
Copy link

Hello,

Thank you for add create/edit bill API in LibreNMS.

In LibreNMS create bill API "https://docs.librenms.org/#API/Bills/#create_edit_bill", there is an field name like "bill_custid". Please let me now, where from i will get this bill_custid?

Best Regards
Harpreet Singh

@richardlawley
Copy link
Contributor Author

The Create/Edit bill API was not added in this PR (or by me), and this is not the correct location to ask questions like this. The best place to ask questions is the discord board: https://t.libren.ms/discord

However, pretty sure that field is just free text and corresponds with the "Customer Reference" field in the web form.

@librenms librenms locked and limited conversation to collaborators Apr 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants