search

teslamate-org / teslamate

A self-hosted data logger for your Tesla 🚘
https://docs.teslamate.org
MIT License
6.04k stars 752 forks source link

Grafana 11.2.3, welcome dashboard, performance improvements, quality / consistency improvements, (re)allow dashboard editing #4338

Closed swiffer closed 2 weeks ago

swiffer commented 3 weeks ago

@JakobLichterfeld - if you prefer seperate branches for some of the commits please ping me - this is a series of improvements as well as Grafana 11.2.3 incl. workaround for fit to data in maps.

@JakobLichterfeld, CHANGELOG needs some love :) you started linking to commits, should we continue doing that?

Welcome Dashboard Screenshot

grafik

netlify[bot] commented 3 weeks ago

Deploy Preview for teslamate ready!

Name Link
Latest commit d9cd26f6a6422ec6bda4e73d7415e218bf69ec02
Latest deploy log https://app.netlify.com/sites/teslamate/deploys/6729f99f4a588a0008afb565
Deploy Preview https://deploy-preview-4338--teslamate.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

JakobLichterfeld commented 3 weeks ago

Thank you for your work and efforts!

if you prefer seperate branches for some of the commits please ping me

Smaller pull requests are easier to test and much easier to revert. So in general I vote for smaller pull requests, and I know there are exceptions, see our recent nix and CI rework #4219 .

CHANGELOG needs some love :) you started linking to commits, should we continue doing that?

😄 I only link commits when I push directly to main. This is kind of bad practice (pushing directly to main), but due to our sec protections in place it is in some cases the most efficient way to fix CI issues. So in this case, I would link to the PR in several places in the changelog.

Before merging: not only do we need to test it, we also need to make sure our nix part works. It seems to work from a quick look at https://github.com/teslamate-org/teslamate/blob/d4a2718085e2e36334e2f3f720d9d7e02cd779de/nix/module.nix#L257, but we need to be sure not to lose the new dashboard when installing via nix.

swiffer commented 3 weeks ago

Nix part -> wasn't aware of that. As the dashboard is part of the internal folder that should work. However need to change editable to allowUiUpdates there as well.

Ok, will do smaller ones in the future, reverting should still be okay if not squashing commits on merge.

Needs a bit of rebasing instead then.

JakobLichterfeld commented 3 weeks ago

I edited the PR to link the corresponding GitHub issues.

swiffer commented 3 weeks ago

updated the module.nix (was missing some other recent additions to the docker file as well and typo fixed)

I cannot test that here, please verify it's working correctly

sdwalker commented 3 weeks ago 
* [cda0c45](https://github.com/teslamate-org/teslamate/commit/cda0c451026b4bf3ffa669b1db2873a09a81136b) - replace all hard-coded case when statements for km -> miles calc with convert_km @sdwalker, could you review this one?

Derived efficiencies are off Can the mi columns be renamed to Distance?

Before: image

After: image

swiffer commented 3 weeks ago

Derived efficiencies are off

thanks, fixed!

Can the mi columns be renamed to Distance?

which columns do you mean by that?

sdwalker commented 3 weeks ago

Can the mi columns be renamed to Distance?

which columns do you mean by that?

Efficiency: image

Trip: image

Rename kW to Power in Trip? image

Probably should be another pull request to fix the others Dutch Tax mi column -> Distance

Drive Stats mi driven -> Distance driven Average kWh used per day -> Average energy used per day

Drives kWh column -> Energy

Overview Charging kW -> Charging Power

Timeline kWh column -> Energy

JakobLichterfeld commented 2 weeks ago

I cannot test that here, please verify it's working correctly

tested

swiffer commented 2 weeks ago

Can the mi columns be renamed to Distance?

which columns do you mean by that?

Efficiency: image

Trip: image

Rename kW to Power in Trip? image

Probably should be another pull request to fix the others Dutch Tax mi column -> Distance

Drive Stats mi driven -> Distance driven Average kWh used per day -> Average energy used per day

Drives kWh column -> Energy

Overview Charging kW -> Charging Power

Timeline kWh column -> Energy

yes, opened a new issue for that - I haven't touched column titles, let's change this after this PR

JakobLichterfeld commented 2 weeks ago

Tested successfully.

  • da8a583 - new dashboard that is configured to be the home dashboard within grafana - it changes the grafana blog and welcome section with a beautiful image (hosted at tesla.com)

Tested successfully. Nice enhancement!

  • e3f6b16 - Grafana 11.2.3 as we cannot go to 11.3.0 for now, however i would like to keep the 11.3 PR small and some things out

Tested successfully.

  • 8856e6d - simplify sql in statistics dashboard (in line with other dashboards now). i wonder why that was needed, @DrMichael, could you review please?

Tested successfully, side by side with v1.31.1

  • cda0c45 - replace all hard-coded case when statements for km -> miles calc with convert_km @sdwalker, could you review this one?

  • 0ee54f0 - ensure date_trunc is always applied to timestamp with timezone, fixes: Statistics Dashboard - Time Driven is Truncated #4268

  • b37c480 - use open street map for visited (same for all dashboards) @DrMichael, how long does it take to load?

Really nice alignment wiht the look and feel of other dashboards. I guess we need to do a batch update of the screenshots in the docs and in readme

  • a748c6e - adds latest github tags for telsamate-org/teslamate to welcome screen - sadly functionality of rss reader is limited currently

Only point I do not like, is the "invalid date". I assue this is due to RSS:

image

  • 080106d - ensure module.nix contains all settings of grafana dockerfile

tested

JakobLichterfeld commented 2 weeks ago

What seems to be broken:

Charges: v1.31.1 image

this pr: image

Interesting: I do not see a rule of the truncat: image

Hovering reveals the whole value

JakobLichterfeld commented 2 weeks ago

Thanks for adding the No in the Charging stats: image

before it was a bit hard: image

But now the bar diagram is not shown as the column gets to thin.

JakobLichterfeld commented 2 weeks ago

Trip:

"select last three drives" is broken. Url changes correctly, but as we do not open a new tab anymore the side does not get refreshed

JakobLichterfeld commented 2 weeks ago

Charge details:

v1.31.1: image

this pr: image

--> no label on x-axis anymore, diagram shifted

JakobLichterfeld commented 2 weeks ago

Wait, what is this image

😆 that's not my expectation.

swiffer commented 2 weeks ago

@JakobLichterfeld adressed your findings (except https://github.com/teslamate-org/teslamate/pull/4338#issuecomment-2453341052). will do later together with finding of sdwalker.

swiffer commented 2 weeks ago

living in the future, that happens when you drive a tesla long enough 😆

grafik

JakobLichterfeld commented 2 weeks ago

What seems to be broken:

Charges:

resolved

JakobLichterfeld commented 2 weeks ago

But now the bar diagram is not shown as the column gets to thin.

Solved image

JakobLichterfeld commented 2 weeks ago

Trip:

"select last three drives" is broken. Url changes correctly, but as we do not open a new tab anymore the side does not get refreshed

fixed

JakobLichterfeld commented 2 weeks ago

@JakobLichterfeld adressed your findings (except #4338 (comment)). will do later together with finding of sdwalker.

Perfect, ty!

JakobLichterfeld commented 2 weeks ago

If I'm right, the only open point is this:

  • 8856e6d - simplify sql in statistics dashboard (in line with other dashboards now). i wonder why that was needed, @DrMichael, could you review please?

@DrMichael Can we get your thoughts on this?

swiffer commented 2 weeks ago

I think he commented already but the comment got lost by force push - sorry for your double work @DrMichael ...

I'll continue working after this PR is merged but wait until then to not make it bigger and bigger

@JakobLichterfeld - removed the reference to - #4268, not fixed as part of this pr, will be added in the next one!

JakobLichterfeld commented 2 weeks ago

So we are ready to merge. Aren't we?

swiffer commented 2 weeks ago

i had no chance yet to read the comment of @DrMichael but I would say yes. I'll take care of reverting in case he has concerns over the simplification

JakobLichterfeld commented 2 weeks ago

Rebase and merge, innit? As we didn't have "create merge commit" enabled and we do not want to squash this pr

swiffer commented 2 weeks ago

will do

swiffer commented 2 weeks ago

@JakobLichterfeld lgtm

DrMichael commented 2 weeks ago

i had no chance yet to read the comment of @DrMichael but I would say yes. I'll take care of reverting in case he has concerns over the simplification

Oh, I just commented, that I introduced $timezone to get the day showing up from 00:00 until 24:00 and not to show the day as in UTC. The statistics for day looked good for me with this PR, but I have no drives or charges, which are in one day with UTC and in another with my timezone. I am just on UTC+1.

So, I suggested to go ahead, if something shows up with someone with a bigger difference, we can easily revert.

JakobLichterfeld commented 2 weeks ago

perfect, ty.