Incorrect Timezone for Value Metrics

[mislam]

• Laravel Version: 6.3.0
• Nova Version: 2.5.0
• PHP Version: 7.3.4
• Database Driver and Version: MySQL 5.7.25
• Operating System and Version: macOS 10.14.5
• Browser type and version: Google Chrome
• Reproduction Repository: None

Description

The Value Metrics does not honor user's browser timezone set from boot() method in NovaServiceProvider. But it does that for the Trend Metrics.

The total number from the trend metrics (i.e. for 7 days) are not matching with the value metrics (i.e. for 7 days). The timezone still seems off. Note that, I set a custom timezone under the boot() method for NovaServiceProvider using Nova::userTimezone. My app's timezone is UTC, Nova's timezone is set to Asia/Dhaka (GMT+6) and my browser's timezone is America/New_York. I noticed, the value metrics' time-tracking starts from 7:00 AM instead of 12:00AM.

config/app.php

'timezone' => 'UTC',

app/Providers/NovaServiceProvider.php

public function boot()
{
    parent::boot();

    Nova::userTimezone(function (Request $request) {
        return 'Asia/Dhaka';
    });

    Nova::style('theme-overrides', public_path('css/nova.css'));
}

app/Nova/Metrics/NewUsers.php

public function ranges()
{
    return [
        'TODAY' => 'Today',
        ...
}

Screenshot

The screenshot was taken on October 15, 2019 at 10:57 PM at New York's timezone. I checked in the database. The trend metrics shows the actual data, while the value metrics data isn't correct.

[jbrooksuk]

Hey @mislam, can you confirm that you have published Nova assets when updating?

[mislam]

@jbrooksuk Yes I did publish and cleared view cache as explained on the upgrade guide. I did the publish on my Local Valet and then pushed the changed to Git, then deployed on Forge. Do I need to manually SSH to the server and publish?

[jayjfletcher]

I am having the same issue since upgrading. Newly seeded db table, value metric shows 0 created.

Timestamps are UTC in the DB, but the query being run is using local timezone.

Payload being sent

Query being run

Timestamps in DB

[jayjfletcher]

Altering the value aggregate method to something like this worked for me...

/**
     * Return a value result showing the growth of a model over a given time frame.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Illuminate\Database\Eloquent\Builder|string  $model
     * @param  string  $function
     * @param  string|null  $column
     * @param  string|null  $dateColumn
     * @return \Laravel\Nova\Metrics\ValueResult
     */
    protected function aggregate($request, $model, $function, $column = null, $dateColumn = null)
    {
        $query = $model instanceof Builder ? $model : (new $model)->newQuery();

        $column = $column ?? $query->getModel()->getQualifiedKeyName();

        $timezone = $request->timezone;

        $timezoneShift = (function (Carbon $dateTime){
            return $dateTime->setTimezone(config('app.timezone'));
        });

        $previousRange = $this->previousRange($request->range, $timezone);
        $currentRange = $this->currentRange($request->range, $timezone);

        $previousValue = round(with(clone $query)->whereBetween(
            $dateColumn ?? $query->getModel()->getCreatedAtColumn(),
            array_map($timezoneShift,$previousRange)
        )->{$function}($column), $this->precision);

        return $this->result(
            round(with(clone $query)->whereBetween(
                $dateColumn ?? $query->getModel()->getCreatedAtColumn(),
                array_map($timezoneShift,$currentRange)
            )->{$function}($column), $this->precision)
        )->previous($previousValue);
    }

[ndberg]

For me its the other way around, the Value metric is correct while the Trend metric is incorrect.

@mislam why do you have different time zones in app & nova?

[Zen0x7]

I have userTimeZone setted to 'America/Santiago'. And my trend metrics doesn't works correctly:

The last value, the time is 20:15. (That's not correct)

The second value, the time is 23:05. (That's correct)

😕

[jbrooksuk]

Timezones are hard haha.

Okay, so I need to know exactly what's going on here. How are trends working in place and not another?

[jayjfletcher]

@jbrooksuk

Timezones are hard haha.

Okay, so I need to know exactly what's going on here. How are trends working in place and not another?

Trend's aggregate function is using Chronos::now(), without a timezone being passed to it. So it must be using the server's default, which could be different from the application timezone. (should this use Carbon and the framework's now helper? )

Value's aggregate function is using Carbon via the now helper, with the users time zone.

Neither Trend nor Value are converting to the application's timezone before running the query. Which is what the db date columns will be saved in. This is where things actually go wrong.

The code I shared above for Value's aggregate sets the timezone to the application's timezone just before running the query.

For Trend something like this would be needed:

/**
     * Return a value result showing a aggregate over time.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Illuminate\Database\Eloquent\Builder|string  $model
     * @param  string  $unit
     * @param  string  $function
     * @param  string  $column
     * @param  string  $dateColumn
     * @return \Laravel\Nova\Metrics\TrendResult
     */
    protected function aggregate($request, $model, $unit, $function, $column, $dateColumn = null)
    {
        $query = $model instanceof Builder ? $model : (new $model)->newQuery();

        $timezone = $request->timezone;

        $expression = (string) TrendDateExpressionFactory::make(
            $query, $dateColumn = $dateColumn ?? $query->getModel()->getCreatedAtColumn(),
            $unit, $timezone
        );

        $possibleDateResults = $this->getAllPossibleDateResults(
            $startingDate = $this->getAggregateStartingDate($request, $unit),
            $endingDate = Chronos::now(),
            $unit,
            $timezone,
            $request->twelveHourTime === 'true'
        );

        $wrappedColumn = $query->getQuery()->getGrammar()->wrap($column);

        $results = $query
            ->select(DB::raw("{$expression} as date_result, {$function}({$wrappedColumn}) as aggregate"))
            ->whereBetween($dateColumn, [$startingDate->setTimezone(config('app.timezone')), $endingDate->setTimezone(config('app.timezone'))])
            ->groupBy(DB::raw($expression))
            ->orderBy('date_result')
            ->get();

        $results = array_merge($possibleDateResults, $results->mapWithKeys(function ($result) use ($request, $unit) {
            return [$this->formatAggregateResultDate(
                $result->date_result, $unit, $request->twelveHourTime === 'true'
            ) => round($result->aggregate, 0)];
        })->all());

        if (count($results) > $request->range) {
            array_shift($results);
        }

        return $this->result()->trend(
            $results
        );
    }

For now, I've been using both fixes as traits in my metrics.

[mislam]

@jayjfletcher I think you might be right. Although I don't know the insight of the code, but what I see in my app is that the trend metrics is using the server's default timezone (from config/app.php) instead of what's set in NovaServiceProvider (as I mentioned in this original issue). You should not question the end-developer why they are not using the same timezone for both app config and for Nova. Rather you should fix the aggregate function so that it respects Nova's timezone.

[jayjfletcher]

The db query itself should use the timezone in app.timezone because that's the timezone any date columns will be based on in the db. Saved dates won't be based on the nova user timezone. And using a timezone in the query that is different than what the columns are based on will throw off any queries.

But anything outside of the actual query, yes respect whatever user timezone is set. This is were any conversions should take place. And looking at the code, I believe the user timezone is respected outside of the query.

If you want to test it out, take the two methods above, turn them into traits, and then use them on your respective metrics. I'm confident you'll get the results you're expecting.

[Zen0x7]

For some reasons, idk why 🤷‍♂️, my value metrics works correctly. Considering that if the timezone is changed using user timezone settings, the metrics will receive the corresponding parameter. Trend and Value metrics receive that parameter ("America/Santiago", "Asia/Dhaka", etc) as string. Specifically, Value metrics has all date respecting that parameter. In Trend metrics, instead, has an unexpected behavior. See the following code:

   ... TrendDateExpression.php

    /**
     * Get the timezone offset for the user's timezone.
     *
     * @return int
     */
    public function offset()
    {
        if ($this->timezone) {
            return (new DateTime(Chronos::now()->format('Y-m-d H:i:s'), new DateTimeZone($this->timezone)))->getOffset() / 60 / 60;
        }

        return 0;
    }

In before code, Chronos::now() generate a date considering the server timezone to create a DateTime instance using the timezone parameter. In my case, the server is configured using "America/Santiago". if you make a request using the parameter as "America/Santiago", this function calc -3. Understanding that it should return a zero. Well, some change i made on this function in order to get correctly the offset.

    /**
     * Get the timezone offset for the user's timezone.
     *
     * @return int
     */
    public function offset()
    {
        if ($this->timezone) {
            return round(\Carbon\Carbon::parse(now($this->timezone)->format('Y-m-d H:i:s'))->diffInSeconds(now($this->timezone)) / 60 / 60);
        }

        return 0;
    }

Chronos was replaced with now helper and timezone was added. Round method was added too because the diffInSeconds function returns a float. Maybe comparing using that method isn't the best way, but solve my issue.

[mislam]

Just found another discrepancy in the currentRange and previousRange methods. Correct me if I'm wrong. See how the now() function uses the timezone. In my test, I was convinced to change the code to be as follows:

[bolechen]

Have same confused here. i found when the mysql-server default timezone is not UTC, at TrendDateExpression.php line 63

return (new DateTime(Chronos::now()->format('Y-m-d H:i:s'), new DateTimeZone($this->timezone)))->getOffset() / 60 / 60;

The ->getOffset() will return wrong value.

[bolechen]

add this macro will tmp fix this:

TrendDateExpressionFactory::macro('mysql', function ($query, $column, $unit, $timezone) {
    return new MySqlTrendDateExpression($query, $column, $unit, 'UTC');
});

[bolechen]

And i see laravel have an not document config database.connections.mysql.timezone, seems nova not used this.

[ifaniqbal]

In my case, trend metric jumps to the next day too soon. I set my timezone in Asia/Jakarta, which is UTC+7, and trend metric already showing trend for the next day at 5pm.

[davidhemphill]

So I'm taking a look into this today 👋.

My understanding of the issue is that when Nova's metrics are calculated, they use the timezone of the user's browser. So if you have records that are in the future in UTC time relative to your local time, you will not see them in the aggregate results. I think this makes sense actually, even though it's confusing.

This confusing behavior was probably @taylorotwell's motivation for adding the Nova::userTimezone(callback). The only problem is we don't use that when calculating metrics either. I think we probably should.

[ifaniqbal]

Trend metric still error with version 2.10.1.

[matthewjumpsoffbuildings]

Any news on this? Im still getting incorrect trend results for a nova app running in the Sydney timezone

[kayacekovic]

My DB is UTC and Laravel application is UTC+3.

This is the query a simple trend produce:

select date_format(`created_at` + INTERVAL 3 HOUR, '%Y-%m-%d') as date_result,
       count(`leads`.`id`)                                     as aggregate
FROM `leads`
WHERE `created_at` between '2020-02-28 00:00:00' and '2020-03-05 21:04:57'
  AND `leads`.`deleted_at` is null
group by date_format(`created_at` + INTERVAL 3 HOUR, '%Y-%m-%d')
order by `date_result` asc;

Dates are correct. Problem is with WHERE created_at...

So I've changed the query as following:

WHERE DATE_ADD(created_at, INTERVAL 3 HOUR) between '2020-02-28 00:00:00' and '2020-03-05 21:41:24'

And it worked!

I had to manipulate src/Metrics/Trend.php line 498.

->whereBetween(\DB::raw('DATE_ADD('.$dateColumn.', INTERVAL 3 HOUR)'), [$startingDate, $endingDate])

[cwilby]

I don't think timezone conversion is necessary for value metrics - (willing to be wrong).

When querying value metrics, Nova's UI requests a count from /nova-api/metrics/{metric}?timezone=America%2FLos_Angeles.

For a value metric using the count method, Nova needs to get the start/end dates for the current and previous range.

The problem - the dates used for these ranges are based on the current moment in the given timezone. (These are calculated by the previousRange and currentRange methods in Laravel\Nova\Metrics\Value).

Removing abstraction, given the following database, these are the queries that are currently being called by Nova.

When in -0800, the user gets 16 hours worth of data 8 hours late. When in +0200, the user gets 22 hours worth of data immediately.

database

{
  users: [
    { id: 1, created_at: '2020-04-19 03:09:40' } // Today in UTC
  ]
}

query in -0800

$previousCount = with(clone $query)->whereBetween(
  'created_at',
  [
    "2020-02-18 20:17:04",    // 2 months ago in PST
    "2020-03-19 20:17:04".    // 1 month ago in PST 
  ]
);

$currentCount = with(clone $query)->whereBetween(
  'created_at',
  [
    "2020-03-19 20:17:10",     // 1 month ago in PST
    "2020-04-18 20:17:10".     // Today in PST, not including Today in UTC.
  ]
);

query in +0200

$previousCount = with(clone $query)->whereBetween(
  'created_at',
  [
    "2020-02-19 05:17:04",    // 2 months ago in +0200
    "2020-03-20 05:17:04".    // 1 month ago in +0200
  ]
);

$currentCount = with(clone $query)->whereBetween(
  'created_at',
  [
    "2020-03-20 05:17:10",     // 1 month ago in +0200
    "2020-04-19 05:17:10".     // Today in +0200, not including 2 hours from yesterday in UTC.
  ]
);

[cwilby]

Setting Nova::userTimezone doesn't solve the issue, as Nova attaches the browsers detected timezone when querying for value metrics.

[cwilby]

When I remove all timezone conversions from Laravel\Nova\Metrics\Value, everything works for the test cases I provided.

[ifaniqbal]

Can you share the patch for it? I want to try it. Thanks

On Sun, 19 Apr 2020 at 10.40 Cameron Wilby notifications@github.com wrote:

When I remove all timezone conversions from Laravel\Nova\Metrics\Value, everything works for the test cases I provided.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/laravel/nova-issues/issues/2044#issuecomment-616019967, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQZEQL7474HP5TECTSOHTTRNJXBNANCNFSM4JBFMEWA .

[cwilby]

@davidhemphill - wanted to make sure I address this point also.

My understanding of the issue is that when Nova's metrics are calculated, they use the timezone of the user's browser. So if you have records that are in the future in UTC time relative to your local time, you will not see them in the aggregate results. I think this makes sense actually, even though it's confusing.

If I have records in the future in UTC time, I would not see them in the aggregate results as long as the range value calculations and database values are using the same timezone.

[cwilby]

Hey @ifaniqbal, did this patch resolve your issue?

[ifaniqbal]

Sorry, I just remembered that the problem I experienced was in the trend metric, not the value metric.

On Wed, Apr 22, 2020 at 11:54 PM Cameron Wilby notifications@github.com wrote:

Hey @ifaniqbal https://github.com/ifaniqbal, did this patch resolve your issue?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/laravel/nova-issues/issues/2044#issuecomment-617901625, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQZEQMIRL56RA4VEA7CAILRN4OM7ANCNFSM4JBFMEWA .

[cwilby]

@ifaniqbal I'll try refactoring the same aggregate function in the Trend class.

If you want to try the patch yourself, the goal is to test what happens by removing timezone conversion logic. It's mostly doing find all for "$timezone", removing code, and checking results in Nova.

[smartens80]

Any progress with this by any chance?

[cwilby]

@smartens80 I started a private fork with timezone conversions removed from metric classes and it works well.

Here's Laravel\Nova\Metrics\Value refactored without timezone conversion

https://gist.github.com/cwilby/6a63eb44bf2e89759410b640c757035f

[GDanielRG]

Such a shame that this is part of the core feature of a paid product and 3 years later that feature cannot be used properly. I just wasted 3 days of my time trying to figure out why my metrics where not working blaming the configuration and coding on me, to find out its a problem with the product itself.

[oceanapplications]

This is still an ongoing problem. My server time is America/New_York, config.app.timezone is America/New_York, my Nova boot method is setting the timezone to America/New_York.

The query generated is: select date_format(`created_at`, '%Y-%m-%d') as date_result, count(`orders`.`id`) as aggregate from `orders` where `created_at` between '2020-10-13 00:00:00' and '2020-10-19 12:35:46' and `orders`.`deleted_at` is null group by date_format(`created_at` - INTERVAL 4 HOUR, '%Y-%m-%d') order by `date_result` asc

The problem is the INTERVAL 4 HOUR being added when the data is already timezone adjusted. I end up with data from 2020-10-12 and that kills the charts.

[adebonis]

Still an issue - were building an important Covid Vaccine signup app and really could use this fixed

[crynobone]

It's extremely hard to follow the replies above:

• Original issues are more than 1 year old.
• Some reply with INTERVAL SQL, which only exists on Trend metric, and not Value Metric as the title. Both should be debugged and reproduce separately.

I can look into it tomorrow but would be useful if each person submits a separate issue with reproducing examples so we can understand each use case.

[crynobone]

This has been fixed and released in v3.21.0

[github-actions[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.