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

Option to save log files with a click on the button in the admin page #584

Merged
merged 8 commits into from
Oct 26, 2016

Conversation

joyrider3774
Copy link
Contributor

@joyrider3774 joyrider3774 commented Oct 25, 2016

As per request of @glynhudson i added a quick option to download the log file. I intentionally did not put text as content type so you always get a download box (depending on the browser) to choose where to save the file and don't view the file inside the browser.

I tested it on my windows and it seems to work fine for me should be tested on linux

add header("Pragma: no-cache"); header("Expires: 0");
remove odbstart & clean
@joyrider3774
Copy link
Contributor Author

joyrider3774 commented Oct 25, 2016

I added 2 extra options to download the emonpi update log and backup log for the emonpi update log i added a button but for backuplog i did not (but did add the route) as i could not find where it is being used.

Please test this on an actual emonpi as i don't have an emonpi i blindly programmed the options dealing with emonpi (the normal log download i did test) !

…st. If not add the file downloaded will contain the message that it did not exist (to prevent blank pages and always downloading a file)
@joyrider3774
Copy link
Contributor Author

for the emonpi related logfiles i added $session['admin'] verifications as i saw the normal getlog routes did this as well. I also added a fileexists check when going the download route and when the file does not exist it still downloads a file with a message in it saying the logfile did not exist. This is to prevent the browser from navigating away when no file would be downloaded or displaying blank pages. (It makes sure a file will always be downloaded even if the log does not exist) The normal logfile does not have this problem as a file is always created if we have write access and if we don't have write access no button is being shown but the emonpi buttons should always be shown as you trigger the update itselve without reloading the page (ajax call)

@glynhudson
Copy link
Member

glynhudson commented Oct 25, 2016

Yah! The logs download great 👍

However, when I open the emonPi log files there seems to be some invalid characters at the start

selection_103

I don't see these characters when I view the log directly on the Pi:

selection_104

No issue with emoncms.log, I can view that perfectly in gedit.

@JonMurphy
Copy link
Contributor

glyn - those have always been there in the emonpiupdate.log file

screen shot 2016-10-25 at 12 52 00 pm

@glynhudson
Copy link
Member

Ah, interesting. So it's no fault of the download.

I wonder where those characters are coming from? I can't see them in the emoncms logview or when I view the log directly

head ~/data/emonpiupdate.log 
Starting emonPi Update >
EUID: 1000

Mon 24 Oct 14:55:32 UTC 2016

git pull /home/pi/emonpi
Updating 10e2b76..c3bee09
Fast-forward
 .gitignore                                      |  27 +++++
 .travis.yml                                     |  42 ++++++++

Mmmm..I'm not sure where to look. Could it be the log file format?

@joyrider3774
Copy link
Contributor Author

i could try adding ob_clean back in it should clear the buffer before downloading the file... but that would only work if something already had written data to the buffer somehow (like extra whitespaces after ?> etc) it even seems they are 0 characters so not sure if they would come from extra whitespaces in the php files. I'll try the ob_clean thing. I'll change it immedialty and do the pr this way you can test if that's the problem

@joyrider3774
Copy link
Contributor Author

the zero's might also come from something that "service-runner-update.sh" does but i don't have that file and can't find in the repositories as all the update function does is run that script and pipe the output to a file

@joyrider3774
Copy link
Contributor Author

the reason the log on the admin page does not have those zero characters was because trim got called on the result. I'm explicitly doing the same thing now. But i don't know what kind of side effect this might have if you got a logfile of say 500 mb or 1 gigabyte etc as your passing that amount of data to the trim function then.

Calling trim should fix the zero characters when downloading the file as well though

… file and might give problems passing the value to trim.
@joyrider3774
Copy link
Contributor Author

don't merge / try yet i made a mistake with the trim statements

…the emonpi update & backup load. Emoncms.log doesn't need this.
@joyrider3774
Copy link
Contributor Author

it should work now. I only trim emonpi update & backup log but not emoncms log. The emonpi logs probably stay very small while the emoncms log can be become large and did not have the problem

@joyrider3774
Copy link
Contributor Author

i tested with a raspberry pi and emonSD-03May16 image and as far as i can tell it seems to work. Perhaps someone else can do a final test before actually merging

Copy link
Member

@glynhudson glynhudson left a comment

Choose a reason for hiding this comment

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

working great for me now, no invalid characters. Just made a few minor comments on path variable referencing. Anyone else able to test, @JonMurphy ?

header("Pragma: no-cache");
header("Expires: 0");
flush();
if (file_exists("/home/pi/data/emonpiupdate.log"))
Copy link
Member

Choose a reason for hiding this comment

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

the path for emonpiupdate.log is defined on line 133

https://github.com/joyrider3774/emoncms/blob/ebe7dc607da65ba2fcb5233bc12d8877f2176656/Modules/admin/admin_controller.php#L133

It would be best to reference this variable $update_logfile here rather than hardcode the abs path in another location

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would not work as far as i can tell the variables are only known on that route (in that if statement). I can move them outside the if statement and reference adapt my code as well as the other code that was there before to use the variable. The only not so clean down side is that the variables would be set each time you use the emonpi route action while perhaps never having need for them although i don't really consider that a bad thing as it would not have an impact and then you can reference the variables

the code actually used harcoded absolute paths before i made my changes ..

if (file_exists("/home/pi/data/emonpiupdate.log"))
{
ob_start();
readfile("/home/pi/data/emonpiupdate.log");
Copy link
Member

Choose a reason for hiding this comment

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

same here

}
else
{
echo("/home/pi/data/emonpiupdate.log does not exist!");
Copy link
Member

Choose a reason for hiding this comment

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

and here

@glynhudson
Copy link
Member

emoncms.log can only be a max of 1Mb (on an emonPi) due to an aggressive log rotate since we are keeping the log on a tmpfs ramdisk. I don't think the log will be too large for trim to handle. The emonpi update log also won't be very large at all few Kb max probably.

https://github.com/openenergymonitor/emonpi/blob/master/logrotate.conf#L12

Working great, just made a couple of minor comments with regard to variable referancing.

…e in other subroutes and grouped all file related references for the emonpi route together
@glynhudson glynhudson merged commit df9e0b0 into emoncms:master Oct 26, 2016
@glynhudson
Copy link
Member

Perfect, thanks. Merged 👍

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.

3 participants