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

Metric repository perf #2040

Merged
merged 3 commits into from
Oct 30, 2016
Merged

Metric repository perf #2040

merged 3 commits into from
Oct 30, 2016

Conversation

jbrooksuk
Copy link
Member

@jbrooksuk jbrooksuk commented Aug 8, 2016

Closes #1900.


Refactoring metrics. With this, I intend to fix the performance of metrics across all three database drivers.

Note that this is probably going to end up bigger than I intended it to be, but it'll make it easier to deal with.

Todo:

  • Refactor all metric repositories to query all metric points between a given time span.
  • Stop using static table names as the tables may be prefixed.

@jbrooksuk jbrooksuk added this to the V2.4.0 milestone Aug 8, 2016
@jbrooksuk jbrooksuk self-assigned this Aug 8, 2016
use CachetHQ\Cachet\Repositories\Metric\MySqlRepository as MetricMySqlRepository;
use CachetHQ\Cachet\Repositories\Metric\PgSqlRepository as MetricPgSqlRepository;
use CachetHQ\Cachet\Repositories\Metric\SqliteRepository as MetricSqliteRepository;
use CachetHQ\Cachet\Repositories\Metric\MySql as MetricMySqlRepository;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like these renames. ;(

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping it how it was. :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, okay then. I'll rename them once it's finished.

public function getPointsSinceMinutes(Metric $metric, $minutes)
{
$queryType = $this->getQueryType($metric);
$points = Collection::make(DB::select("SELECT DATE_FORMAT(metric_points.`created_at`, '%H:%i') AS `key`, {$queryType} FROM {$this->getTableName()} INNER JOIN metric_points ON metrics.id = metric_points.metric_id WHERE metrics.id = :metricId AND metric_points.`created_at` >= DATE_SUB(NOW(), INTERVAL :minutes MINUTE) GROUP BY HOUR(metric_points.`created_at`), MINUTE(metric_points.`created_at`) ORDER BY metric_points.`created_at`", [
Copy link
Contributor

Choose a reason for hiding this comment

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

These are absolutely disgusting... 😱

Copy link
Contributor

Choose a reason for hiding this comment

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

:trollface:

Copy link
Member Author

Choose a reason for hiding this comment

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

Laravel 5.3 stops us having to wrap DB::select in a collection at least.

@jbrooksuk
Copy link
Member Author

The issue we have with all of the metrics is that each DB driver queries differently and Eloquent isn't flexible enough to allow us to keep the same query structure — unless I'm missing something?

I really want to get this into 2.4, so I'll try and finish this at a later date.

@jbrooksuk
Copy link
Member Author

Rebased.

@jbrooksuk jbrooksuk changed the title [WIP] Metric repository perf Metric repository perf Oct 30, 2016
@jbrooksuk
Copy link
Member Author

This is finally done!

@jbrooksuk jbrooksuk merged commit fbb59ea into 2.4 Oct 30, 2016
@jbrooksuk jbrooksuk deleted the metric-repository-perf branch October 30, 2016 17:36
@ConnorVG
Copy link
Contributor

😱 👍

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

Successfully merging this pull request may close these issues.

Non-optimal app layer iteration in MetricRepository
3 participants