3
\$\begingroup\$

The following code is one of a few similar snippets that generate a graph using CanvasJS, with the datapoints for the graph pushed to an array in PHP during a while loop. The code pulls the the date and the escalation event toals for every weekday for the last 30days, does some math on them, and then pushes the calcualted outcome Y value into an array with the dayID/date as the X value/label. The array is then JSON encoded to be used by CanvasJS.

Currently, this particular page (and hence this graph) takes around 9 seconds to run.

Disclaimer: I am not a PHP developer, and I know very little about what is the correct way to do this. This code is my way of doing it, however I know how. I am fully expecting this to be torn apart, haha.

UPDATE: A little info on what the graph shows, and why there are 2 DB's. The graph shows a percentage figure for each day that is derived from the total number of KPI breachable events in a ticket system, and the number of KPI Breach events that day for tickets in the ticket system. It's fairly high volume, and each day could contain 100-400 events. There are 2 databases because the KPI breaching system was made by me, I track events and the status they are in, and log how long they are in that status for, if that time goes above a defined limit, it is classed as breached (or escalated in this context). I store all of that KPI breach tracking information in a different database, as I cannot add tables or otherwise manipulate the database that contains the ticket information/data (it's an ISMS controlled system, and is a managed/supported solution form a 3rd party, I cant change their DB structure.).

So to summarise that, $db is the ticket system, whcih pulls the total number of escalatable events for a day (and the date), and $db2 is my own DB containing escalated events, timestamps and whether or not they've recovered from escalation status/KPI breach.

<div id="chartContainer" style="height: 370px; width: 800px;"></div>
<script src="https://canvasjs.com/assets/script/canvasjs.min.js"></script>

<?php
//Debug Mode
//
$debug=0;
if($debug == 1) {
  ini_set('display_errors', 1);
  ini_set('display_startup_errors', 1);
  error_reporting(E_ALL);
}

//Get the creds and conn strings for the 2 DBs
//
include("conf/db.php");
include("conf/db2.php");


//Build Graph Data Points
//
$daysmax=31;
$daycounter=0;
$datapoints = array();

while($daysmax > $daycounter) {
  if ($debug == 1) {
    echo "Day: ".$daycounter." -- ";
  }
  $sql_get_day_total_events="SELECT COUNT(*),date_sub(curdate(),INTERVAL $daycounter day) as date1 FROM tblticketlog
    WHERE DATE(date) = date_sub(curdate(),INTERVAL $daycounter day)
    AND dayofweek(date) NOT IN (1,7)
    AND ((action = 'New Support Ticket Opened')
    OR (action LIKE 'Status changed to Pending%')
    OR (action LIKE 'Status changed to In Progress%')
    OR (action LIKE 'New Ticket Response made by%'))";
  $get_day_total_events_result=mysqli_query($db,$sql_get_day_total_events);
  if(mysqli_num_rows($get_day_total_events_result) > 0) {
    while($row = $get_day_total_events_result->fetch_assoc()) {
      $day_result_date=strtotime($row['date1']);
      $day_result_value=$row['COUNT(*)'];
    }
  }
  $sql_get_day_escs="SELECT COUNT(*),date_sub(curdate(), INTERVAL $daycounter day) as date FROM escalations
    WHERE DATE(esc_at) = date_sub(curdate(), INTERVAL $daycounter day)
    AND dayofweek(esc_at) NOT IN (1,7)
    AND escalated = 1";
  $get_day_escs_result=mysqli_query($db2,$sql_get_day_escs);
  if(mysqli_num_rows($get_day_escs_result) > 0) {
    while($row = $get_day_escs_result->fetch_assoc()) {
      $day_escs_value=$row['COUNT(*)'];
    }
  }
  $day_slas_met=$day_result_value - $day_escs_value;
  $day_kpi_pcnt = round((($day_slas_met / $day_result_value) * 100),0);
  if ($debug == 1) {
    echo date("d-m-Y",$day_result_date)." - ".$day_slas_met."/".$day_result_value." - ".$day_kpi_pcnt."%<br />";
  }
  if ($day_result_value > 0) {
    $datapoints[] = [
      'x' => 30 - $daycounter,
      'label' => date("d-m-Y",$day_result_date),
      'y' => $day_kpi_pcnt,
    ];
  }
  $daycounter=$daycounter + 1;
}

$datapoints_rev = array_reverse($datapoints);

if ($debug == 1) {
  print_r($datapoints_rev);
}

?>

<script>
window.onload = function () {

var chart = new CanvasJS.Chart("chartContainer", {
        animationEnabled: true,
        exportEnabled: true,
        theme: "light1",
        title:{
                text: "SLA Met Last 30 Days (Weekdays, %)"
        },
        data: [{
                type: "line",
                //indexLabel: "{y}",
                indexLabelFontColor: "#5A5757",
                indexLabelPlacement: "outside",
                dataPoints: <?php echo json_encode($datapoints_rev, JSON_NUMERIC_CHECK); ?>
        }]
});
chart.render();

}
</script>

If it matters, this is the graph it generates:

canvasjs graph 1

\$\endgroup\$
  • 1
    \$\begingroup\$ A way to speed this up is to get the data for all the days with one query, instead of a separate query for each day. Then GROUP BY date. PS: it would be nice to know what the graph is about and why you need two databases. \$\endgroup\$ – KIKO Software Jul 31 at 12:39
  • \$\begingroup\$ @KIKOSoftware Thankyou for the reply, I've added another section to my post that briefly explains its purpose, and why there are 2 different databases in use. I will investigate pulling all the data in 1 query. I think I did try that originally and it dodnt work how i expected it to. Not that thats a concern for codereview of course.. \$\endgroup\$ – Dave Byrne Jul 31 at 12:55
2
\$\begingroup\$

I first had to look up what KPI stands for. It is a "Key Performance Indicator". Then a "KPI breachable event" must be when some indicator gets over a certain threshold? That's my best guess... Better not use an abbreviation, or jargon, that is unrelated to programming, when explaining what your code is doing. There's a big change we don't know what you're talking about.

There will also be a lot of things I cannot review in this answer, since your question doesn't contain all the code/data/schema. Lack thereof also means I cannot test the code here. Warning: There may be bugs.

Given these restrictions I've tried to rewrite your code so it only does one query on each database for all the days in the graph, thereby possibly speeding up your code by a factor of 30, meaning that it should complete in less than half a second. Which is still quite a long time.

One of the other reasons the queries can be slow is the way you've implemented the action column in the tblticketlog table query. It is a string and uses several LIKE comparisons. The query could be made faster if the status of a ticket can be checked more efficiently. Imagine you had a column called status of type SET with the values ('new', 'rejected', 'pending', 'progress', 'responded', 'delete'), or something similar. The test could then be:

.... AND status IN ('new', 'pending', 'progress', 'responded')

Which is a lot faster, and also shorter. You could still have a column similar to action with more details about the status of the ticket.

Also check whether you have set the proper indexes for these queries.

Here's the rewritten code, without the debugging stuff.

I am going to use this function:

function selectDailyEvents($db, $query)
{
    $data = [];
    if ($result = mysqli_query($db, $query)) {
        while ($row = $result->fetch_assoc()) {
            $data[$row["dayNo"]] = ["date"  => $row["eventDate"],
                                    "count" => $row["eventCount"]];
        }
        $result->free();
    }
    return $data;
}

Because both queries below return, more or less, the same result, I can use the single function above to retrieve the data from the databases. This is how the function is used:

$totalDays = 31;

$query = "SELECT COUNT(*) AS eventCount,
                 DATE(date) AS eventDate,
                 $totalDays - DATEDIFF(date, curdate()) AS dayNo
          FROM tblticketlog
          WHERE DATE(date) > date_sub(curdate(),INTERVAL $totalDays day) AND 
                dayofweek(date) NOT IN (1,7) AND 
                ((action = 'New Support Ticket Opened') OR 
                 (action LIKE 'Status changed to Pending%') OR 
                 (action LIKE 'Status changed to In Progress%') OR 
                 (action LIKE 'New Ticket Response made by%'))
         GROUP BY ticketDate 
         ORDER BY ticketDate";

$dailyTickets = selectDailyEvents($db, $query);

$query = "SELECT COUNT(*) AS eventCount,
                 DATE(esc_at) AS eventDate, 
                 $totalDays - DATEDIFF(date, curdate()) AS dayNo
          FROM escalations
          WHERE DATE(esc_at) > date_sub(curdate(), INTERVAL $totalDays day) AND 
                dayofweek(esc_at) NOT IN (1,7) AND 
                escalated = 1      
          GROUP BY escalationDate   
          ORDER BY escalationDate";

$dailyEscalations = selectDailyEvents($db2, $query);

These two queries are exactly the same ones as you have, except that I added the GROUP BY and ORDER BY. I also added a dayNo in the output, because that indicates which day it is in the sequence.

Now we have two arrays, $dailyTickets and $dailyEscalations, which needs to be rearranged for the graph. We use a loop to go over each day:

$graphData = [];

for ($dayNo = 0; $dayNo < $totalDays; $dayNo++) {
    $ticketCount = 0;
    if (isset($dailyTickets[$dayNo])) {
        $eventDate   = $dailyTickets[$dayNo]["eventDate"];
        $ticketCount = $dailyTickets[$dayNo]["eventCount"];
    }
    $escalationCount = 0;
    if (isset($dailyEscalations[$dayNo])) {
        $eventDate       = $dailyEscalations[$dayNo]["eventDate"];
        $escalationCount = $dailyEscalations[$dayNo]["eventCount"];
    }
    if (isset($eventDate)) {
        $serviceLevel = round(100 * ($ticketCount - $escalationCount) / $ticketCount, 0);
        $readableDate = date("d-m-Y", strtotime($eventDate));
        $graphData[] = ['x'     => $dayNo,
                        'label' => $readableDate,
                        'y'     => $serviceLevel];
    }
}

I'm not sure the result of this last bit of code is exactly what you need, but with a bit of debugging you should be able to get there. As I said before: I cannot test this. The database queries are probably correct.

I've also notice that your coding style is not optimal. Spacing is inconsistent. Also the naming of database columns and PHP variables could be improved. That means escalated_at instead of esc_at and $ticketCount or $escalationCount instead of $day_result_value. Always try to make your code as easy to read as possible.

\$\endgroup\$
  • \$\begingroup\$ Thankyou for all of that, I appreciate the time spent to try and decipher my quite obviously "hacky" code. I didnt realise you'd replied, and actually spent this mornign rewriting most of what I was doign to only query the DB's once each and then loop on the returned rows to push values into the array. I got the PHP processing time down form 9+ seconds to 488ms, whcih I'm happy with considering what I started with. That said, my code is still very different to your suggestions, so I'll make some adjustments and adopt more elements of your suggestions there. Thanks. \$\endgroup\$ – Dave Byrne Aug 1 at 15:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.