Issue #67: Backfilled metric - online users

[marcghaly]

Hi @jaypha, hi @brendanheywood,

This is PR for issue #67.

Regards,

Marc

[jaypha]

There are some issues here which I have concerns about. I'll be happy to discuss if you want to.

[jaypha]

Hi Marc.

I don't think a meeting is necessary. You have addressed my primary concerns for now. I'm a little iffy about having it controlled from within the database collector, but that's something I'd rather leave to Brendan.

Just that one fix causing the CI problem, and then I am OK to merge. Get Brendan to have a look at the UI aspect.

Regards

[marcghaly]

Hi @brendanheywood,

I have made changes for this PR, a concern that I have is the use of the API for retrieving logs that does not provide a faster / lighter way to retrieve concurrent users (ie: using DISTINCT in a SELECT statement - retrieving all data * with the 2 methods provided by the API get_events_select_iterator, get_events_select seems super heavy to me, please let me know if there is a way I am not aware of) - it seems it is taking a lot of time for this operation, if that were to be left as is, would it need a spinner display ? This is rebased against latest MOODLE_35_STABLE. Concerning a meeting, I am available at 8 PM EST so 10 AM AEST your time, this evening and tomorrow if you want.

Regards,

Marc

[brendanheywood]

hi @marcghaly

If the log api is too slow then do it directly like it was. But just to be be clear @jaypha was doing / has just done a huge refactor of the way the metrics work and I didn't want to waste your time as it would affect your patch. I suspect you will need to have a look at what has landed and rewrite a lot of it (despite there being no conflicts)

We have our standup at 10, so lets chat right after that. Can you please ping me on matrix, I pinged you the other day but you didn't see it and I'm not sure I have the real 'you' in matrix

[marcghaly]

Hi @brendanheywood,

I have made the following changes:

• Used metric_item class directly in generate_metric_items instead of re-calling generate_metric_item.
• Added the UI on another page with one input representing period.
• Displayed what data can be retrieved - and what data has already been retrieved.
• Used the interval from the Frequency setting in Manage Metrics panel.
• Added a link to backfill support in the Manage Metrics panel.
• Queried the logstore directly for performance purposes, so it supports mariadb and pgsql.
• Added a renderer/mustache template for chart page.
• Instead of erasing data present in cltr db, when date is further than one already present, only complete data.
• If frequency is changed, data would be erased - it seems confusing having different intervals displayed - can change though.

Let me know if this suits.

Regards,

Marc

[brendanheywood]

hi @marcghaly

This is looking great and pretty close, one issue I've found is that when you backfill data you don't end up with nice time periods which align with time period boundaries:

If we had a setting that was supposed to have been every hour then I'd expect the backfilled data to be on the hour every hour

A few other small comments in the pr

[jaypha]

Hi Marc

If we had a setting that was supposed to have been every hour then I'd expect the backfilled data to be on the hour every hour

In case you didn't know, there are several functions in the classes/lib.php which can help with this.

[marcghaly]

Hi @brendanheywood,

Just checking, in the following example it is set to be hourly, for example between two first rows, should it display between 12:00 and 16:00 (12, 13, 14, 15 , 16) with a 0 value ? Is that what you mean ? I am using pgsql maybe issue is related to mariadb/mysqli

This is what I get with mysql:

Regards,

Marc

[brendanheywood]

Just checking, in the following example it is set to be hourly, for example between two first rows, should it display between 12:00 and 16:00 (12, 13, 14, 15 , 16) with a 0 value ? Is that what you mean ? I am using pgsql maybe issue is related to mariadb/mysqli

The golden rule is imagine that the metric had been running the whole time and we didn't need to back fill. Ideally the end result should be exactly the same. So yes there would be a metric for every time slice and many of them would be 0

[brendanheywood]

Also I am using postgress too

[marcghaly]

Hi @brendanheywood,

I have made changes according to your comments, If changes are okay I will squash commits.

Cheers,

Marc

[marcghaly]

Hi @brendanheywood, this is squashed and rebased over latest master.

Regards,

Marc

[brendanheywood]

thanks @marcghaly