Skip to content

Logging feature #81

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

Merged
merged 27 commits into from
Dec 18, 2015
Merged

Logging feature #81

merged 27 commits into from
Dec 18, 2015

Conversation

mguijarr
Copy link
Member

(sorry for PR #80, I made a mistake with git in my own repo and it was closed automatically)

I implemented a basic logging feature for MXCuBE 3 ; it should close #14 I guess.

What has been done:

  • a new custom log handler is set up when server starts, directly under 'root' logger to be able to aggregate all log messages
  • clients "register" by connecting the new Logging route
  • all records are JSON-encoded and emitted to clients via SSE
  • on the client side, there is a new Logging component
  • it is deliberate that the Logging component is not part of redux, because I don't see the point of centralizing and keeping state for this component, or defining actions
  • as soon as web page loads, a global SSE event listener is created ; if connection to server fails, it should try to reconnect every 5 seconds. If connection is lost afterwards, it tries to reconnect every 30 seconds this was rather cumbersome to do, hopefully it is done right ; any improvement idea is welcome would it make a difference to use the websocket ? I tried SSE because I am much more familiar with it
  • all log records are kept forever (well, until page reloads...) ; at the moment in our desktop version of MXCuBE it is like this it doesn't seem to be a problem but...
  • the Logging component tries to display messages in a fancy way, using pagination: log records are limited to 20 per page ; newest log records are at the end
  • when a log record contains stack trace information (like "Exception" log records), it is possible to click on the message to have it fully displayed

Any comments/feedback would be highly appreciated.

PS: I hate the fact that react-router unmounts / remount components when changing from one page to another but I could not find any workaround, except moving 'System log' out of react-router control. Well, it works so it's ok but in fact this is why I had to make the global event listener it's not very clean IMO

PPS: this PR also deletes a file we don't need anymore (test samples list)

@meguiraun
Copy link
Contributor

hi, some questions:

  • SSE vs Websockets: I thought we were going to use websockets for server/client communication, see User interface and server message handler #6 for the case of HO messages. Those messages should also be part of the logging (and not only python logging) so for me it makes sense to send everything through the same ~channel and define different namespaces, or at least using the same technology.
    See User interface and server message handler #6 and the following code for an example of usage (it will be better than any word)
var socket = io.connect('http://' + document.domain + ':' + location.port+'/test');// + namespace);
socket.emit('my event', {data: 'I\'m connected!'});
socket.on('test', function(msg) {
    console.log(msg)
}
  • For the user interface... perhaps it is all about simple log vs detailed log. This solution covers the detailed log, for me, the detailed log should be decoupled from mxcube (something like sentry, kibana/elasticsearch, etc). I like your approach but we might need to discuss how we all see the place for this information. I think we need persistency on a detailed ~web log and we should use external tools for that. Of course, in the meantime this solution is perfect.

Is it ok if we postpone the acceptance of this PR until we discuss it on Friday?

@mguijarr
Copy link
Member Author

You're right we should use websockets only as we decided that, so I am going to update the PR with websocket instead of SSE.

I agree on the need for an external server to store log and to display log info (my favourite is Sentry) but the deadline on this issue #14 is January so I wanted to provide a solution for the time being.

We can postpone it until Friday, no problem.

@mguijarr
Copy link
Member Author

Finally I managed to make it working with socket.io ! Yeepee.

In fact, it was almost ok since the beginning, but I could not see it working in my environment. Why? Read below if you are interested:

I have the following setup:

  • a debian 7 linux box where I run npm start (i.e. the webpack development server with hot reload etc.)
  • a Windows computer with latest Firefox that I use to display MXCuBE 3 UI, served by the debian 7 linux
  • webpack-dev-server is acting as a proxy for all AJAX requests to mxcube/api/..., thanks to my backend-server.js ; requests are sent to another Linux host on a beamline
  • this beamline computer is debian 6, I cannot easily install node 0.12.7 and npm on it - so I am just running Python code: mxcube3-server
  • it receives all requests from the UI and replies are sent back to the debian 7 computer thanks to webpack-dev-server proxy

It turns out webpack-dev-server proxy doesn't handle websockets properly at the moment (see webpack/webpack-dev-server#283 - it's a long story). In our particular case, with flask-socketio on the server side, even the previous bug fix (webpack/webpack-dev-server#302) is not enough at least I could not do anything to make it work.

It is quite annoying, but I am going to follow what happens and maybe a solution will come in the next weeks ; I really like the possibility to have mxcube3 server running on a beamline computer while I am working on the web application on my debian 7 computer. As it is now, I won't be able to see logging messages via a websocket, it will go through xhr polling at best (gracefully degraded by socketio).

Could we merge this PR now ? Thanks a lot !

PS: I took the opportunity to fix dependencies in package.json to the right versions and I updated the webpack production config.

meguiraun added a commit that referenced this pull request Dec 18, 2015
@meguiraun meguiraun merged commit 5e372c1 into mxcube:master Dec 18, 2015
@mguijarr mguijarr deleted the logging branch June 7, 2016 08:34
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.

Message Logger
2 participants