Code review comment for lp:~adeuring/charmworld/more-heartbeat-info-3

On 13.11.2013 15:24, Benji York wrote:
> Review: Approve
>
>> This branch adds three more details to the heartbeat page:
>
> Very nice. The branch looks good once you have addressed my comments to
> your satisfaction.
>
>> To store the timestamps "server started" and "last ingest job", I
>> added a collection "status" to the mongo DB. I am not sure if this is
>> the right collection name -- "misc" might be better just in case that
>> some container for some other data is needed.
>
> How about "server_status"? That will keep "status" available for
> application data.

Fixed.

>
> We happen to run a single server at the moment, but we should assume
> that in as few places as possible. How about a mapping from hostname to
> last start time, that way we can grow into multiple servers.

Changed. We discussed this on IRC; ISTM that for now the best key to
identify a server is it's hostname. This is not ideal in conjunction
with juju, but it allows at least to look up a machine as seen on the
heartbeat page in "juju status".

Since hostnames may become stale, I also added a script that allows to
manually remove stale entries from the database.

>
>> The time of the last ingest job is added by calling the new function
>> set_status_timestamp() in ingest.run_job(); this required a modification
>> in the unit tests of run_job() and in run_job() because
>> set_status_timestamp() obviously needs a MongoDB instance.
>
>>From line 106 of the diff:
>
> + # Some tests do not set the 'db' attribute.
> + if getattr(job, 'db', None) is not None:
> + set_status_timestamp(job.db, LAST_INGEST_JOB_FINISHED)
>
> It seems that there is only one test that was faking the DB; I wonder if
> introducing this code path is worth it for one test. It seems to me
> that it would be preferable to mutate the test rather than the code.

Right, that's a bit ugly. More than one test is affected though:
test_run_job_defaults(), test_run_job_no_setup(),
test_run_job_db_specified() (here, the second run_job() call fails),
test_run_job_general_exception().

I moved the call of set_status_timestamp() into UpdateCharmJob.run() and
UpdateBundleJob.run(); this makes the "if getattr(...)" unnecessary.

>
> Other thoughts:
>
> The first sentence of the docstring on line 277 spans more than one
> line. I would just make it a comment.

changed.

>
> Should we check the mode bits of the crontab file? I don't know that it
> has ever caused us a problem, but it is a common problem people have
> with crontabs.
>
> In formatted_status_timestamp(), it would be nice to have the time zone
> on the formatted time (i.e., tack on a "Z").

Changed.

>
> I would like to see the already too big test_all_checks_pass() and
> test_checks_fail() broken into one test for each passing/failing
> component, but that's not a blocker.
>