Skip to content
This repository has been archived by the owner on Aug 22, 2022. It is now read-only.

Limit the number of log lines displayed #38

Merged
merged 3 commits into from
Feb 19, 2016
Merged

Limit the number of log lines displayed #38

merged 3 commits into from
Feb 19, 2016

Conversation

omarkhan
Copy link
Contributor

See OC-1035.

Problem

The interface takes a very long time to load, as it tries to fetch all log entries for all instances and there can be a lot of data sent over the wire.

Solution

This pull request adds a configurable limit on the number of log entries returned in the initial request. You can set the limit using the LOG_LIMIT environment variable (default: 10000).

In addition, loading log entries from the server is deferred until the user actually selects an instance.

Testing

To run the js tests in your development environment, see #39.

To test manually, create a server and instance with some log entries. We can use the test factories for this:

from instance.tests.models.factories.server import OpenStackServerFactory

server = OpenStackServerFactory.create()
server.log_entry_set.create(level='INFO', text='server log message')
server.instance.log_entry_set.create(level='INFO', text='instance log message')

Then, start the dev server with

make rundev

With your web console open, go to http://localhost:5000/ and select the instance we just created. You will see a second request to fetch the log entries.

instance_log_entry = next_instance_log_entry()

while server_log_entry is not None:
log.append(server_log_entry)
yield server_log_entry
server_log_entry = next_server_log_entry()
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope: Are we using Python 3.5? If we do, the whole function above should be deleted, and we should use heapa.merge() instead. (In Python 3.4, this function doesn't have a key parameter, but it might be worthwhile to rewrite the function anyway.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually pylint does support python 3.5 now, so we could make the switch. See #6

Copy link
Contributor

Choose a reason for hiding this comment

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

For the time being, let's just leave the code the way it is. While it's easy to write a helper function implements the key parameter around the old heapq.merge(), it's rather cumbersome (and slow) to write something that supports the reversed parameter. For reversing, you really need to implement custom comparisons, and can't simply annotate the objects with their keys.

We can clean this up once we're using Python 3.5.

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 have reverted this change for the time being.

Copy link
Contributor

Choose a reason for hiding this comment

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

@omarkhan Argh, that's not really what I meant (though it's really what I wrote). I actually meant: just leave the code the way you have it.

Anyway, doesn't really matter, either way is fine, and we'll make sure to simplify this once we are on Python 3.5.

@smarnach
Copy link
Contributor

@omarkhan Well done, looks good already! I've added a few comments, and will continue testing this tomorrow.

@omarkhan
Copy link
Contributor Author

@smarnach I have added some manual test instructions. Let me know if you have any problems.

@smarnach
Copy link
Contributor

@omarkhan Tested manually, looks all good now. 👍

@omarkhan
Copy link
Contributor Author

Thanks @smarnach, rebasing

omarkhan added a commit that referenced this pull request Feb 19, 2016
Limit the number of log lines displayed
@omarkhan omarkhan merged commit 186a161 into master Feb 19, 2016
@omarkhan omarkhan deleted the omar/logging branch February 19, 2016 06:32
@omarkhan
Copy link
Contributor Author

@antoviaque will this deploy automatically?

@antoviaque
Copy link
Member

@omarkhan Sadly no, at least not yet - definitely on the list of future improvements: https://opencraft.uservoice.com/forums/280754-opencraft/suggestions/9937191-self-deployment . You will have to do it yourself for now (which you can since updates aren't handled by ansible currently):

  • ssh ubuntu@console.opencraft.com
  • then access screen -rd
  • pull the latest code from master (in a sudo su www-data shell, opencraft lives in /var/www/opencraft)
  • then switch to the screen that runs make run, stop it, run the migrations and restart it.
  • also restart the make shell to ensure it uses the latest code

Let me know if you have any question.

@smarnach
Copy link
Contributor

@omarkhan I have deployed this to production. (It happened automatically when I deployed my changes.) I recommend trying out http://console.opencraft.com/ with this PR installed; it's great! Thanks for getting this done!

@omarkhan
Copy link
Contributor Author

omarkhan commented Feb 21, 2016 via email

@antoviaque
Copy link
Member

@omarkhan Yes, thank you so much for that fix! Really great :D

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

Successfully merging this pull request may close these issues.

3 participants