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

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

Merged
merged 12 commits into from
Nov 5, 2024

Conversation

swiffer
Copy link
Contributor

@swiffer swiffer commented Nov 1, 2024

@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

Copy link

netlify bot commented Nov 1, 2024

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit d9cd26f
🔍 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.

@swiffer swiffer changed the title Grafana 11 2 3 Grafana 11.2.3, welcome dashboard, performance improvements, quality / consistency improvements, (re)allow dashboard editing Nov 1, 2024
@JakobLichterfeld JakobLichterfeld added enhancement New feature or request area:dashboard Related to a Grafana dashboard kind:documentation Adds or improves documentation area:grafana Related to Grafana docker Pull requests that update Docker code labels Nov 2, 2024
@JakobLichterfeld
Copy link
Collaborator

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

provision = {
, but we need to be sure not to lose the new dashboard when installing via nix.

@swiffer
Copy link
Contributor Author

swiffer commented Nov 2, 2024

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
Copy link
Collaborator

I edited the PR to link the corresponding GitHub issues.

@swiffer
Copy link
Contributor Author

swiffer commented Nov 2, 2024

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
Copy link
Contributor

sdwalker commented Nov 2, 2024

* [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
Copy link
Contributor Author

swiffer commented Nov 2, 2024

Derived efficiencies are off

thanks, fixed!

Can the mi columns be renamed to Distance?

which columns do you mean by that?

@sdwalker
Copy link
Contributor

sdwalker commented Nov 2, 2024

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
Copy link
Collaborator

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

tested

@swiffer
Copy link
Contributor Author

swiffer commented Nov 3, 2024

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
Copy link
Collaborator

JakobLichterfeld commented Nov 3, 2024

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?
  • 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
Copy link
Collaborator

JakobLichterfeld commented Nov 3, 2024

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
Copy link
Collaborator

JakobLichterfeld commented Nov 3, 2024

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
Copy link
Collaborator

JakobLichterfeld commented Nov 3, 2024

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
Copy link
Collaborator

JakobLichterfeld commented Nov 3, 2024

Charge details:

v1.31.1:
image

this pr:
image

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

Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld left a comment

Choose a reason for hiding this comment

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

Thank you again for all your hard work and efforts!

Overall lgtm. Some small findings, see my comments on PR.

@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Nov 3, 2024

Wait, what is this
image

😆 that's not my expectation.

@swiffer
Copy link
Contributor Author

swiffer commented Nov 3, 2024

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

@JakobLichterfeld
Copy link
Collaborator

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

Perfect, ty!

Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld left a comment

Choose a reason for hiding this comment

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

lgtm (except #4338 (comment))

@JakobLichterfeld
Copy link
Collaborator

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
Copy link
Contributor Author

swiffer commented Nov 5, 2024

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
Copy link
Collaborator

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

@swiffer
Copy link
Contributor Author

swiffer commented Nov 5, 2024

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
Copy link
Collaborator

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

@swiffer
Copy link
Contributor Author

swiffer commented Nov 5, 2024

will do

@swiffer
Copy link
Contributor Author

swiffer commented Nov 5, 2024

@JakobLichterfeld lgtm

@JakobLichterfeld JakobLichterfeld merged commit 08873c7 into teslamate-org:master Nov 5, 2024
15 checks passed
@swiffer swiffer deleted the grafana-11-2-3 branch November 5, 2024 15:27
@DrMichael
Copy link
Collaborator

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
Copy link
Collaborator

perfect, ty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dashboard Related to a Grafana dashboard area:grafana Related to Grafana docker Pull requests that update Docker code enhancement New feature or request kind:documentation Adds or improves documentation
Projects
None yet
4 participants