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

More information in the System section and also update the api call #149

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mrsaxenanitin
Copy link

Proposed change

Type of change

  • [*] Bugfix
  • [*] New feature
  • Code quality improvements to existing code or addition of tests
  • Documentation

Additional information

Checklist

  • The code change is tested and works locally.
  • The code has been formatted using Black.
  • Tests have been added to verify that the new code works.
  • Documentation added/updated if required.

@mrsaxenanitin
Copy link
Author

image

@googanhiem
Copy link

Testing this pull, very nice work. Not sure if this is due to testing other changes to this integration but this update has created a new device, which is called "System" instead of "TrueNAS System". Otherwise it fixes a couple of the sensors.

@googanhiem
Copy link

googanhiem commented Mar 21, 2024

Also the polling interval might be too high. I turned it on at the red arrow in this pic.

image

On my weak system its taking up quite a few cpu cycles, getting all the data across.

EDIT: I reduced the update interval and its made a significant reduction on the cpu impact. Every 60s seems good enough for home assistant data needs. update_interval=timedelta(seconds=60),

@googanhiem
Copy link

Found another issue, its not parsing the disk identifiers correctly. Should be showing 2318BV401352, but instead its using this in the entity_id field {serial_lunid}2318BV401352 or instead of 0xb2ba8014 it shows {serial}0xb2ba8014

Think its to do with this pull #131

@tomaae
Copy link
Owner

tomaae commented Mar 23, 2024

Adding sensors for each memory value like that just needlessly adds load to HA database. It should be all under Memory attributes like they were before. Swap can be there too.
Changes you made made to API values, would actually break the integration. Which truenas version are you using?

@@ -433,8 +459,8 @@ def get_systemstats(self) -> None:

# arcsize
if tmp_graph[i]["name"] == "arcsize":
tmp_arr = ("cache_size-arc_value", "cache_size-L2_value")
self._systemstats_process(tmp_arr, tmp_graph[i], "memory")
tmp_arr = ("arc_size")
Copy link
Owner

Choose a reason for hiding this comment

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

we want L2arc values for those who use it

if t == "memory":
self.ds["system_info"][tmp_var] = tmp_val
Copy link
Owner

Choose a reason for hiding this comment

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

Why changing how tmp_var works? this just results in unnecessary large code and more difficult maintenance.

@mrsaxenanitin
Copy link
Author

Which truenas version are you using?

Thanks @tomaae for your reply.
I am very new to the github and i only written code for my personal purpose.

I am happy that you reviewed it and appreciate your changes.

I am currently using the Beta version 24.04 and yesterday updated to RC1.

I am happy to see your updated repo.

Great work.

@mrsaxenanitin
Copy link
Author

Which truenas version are you using?

Thanks @tomaae for your reply. I am very new to the github and i only written code for my personal purpose.

I am happy that you reviewed it and appreciate your changes.

I am currently using the Beta version 24.04 and yesterday updated to RC1.

I am happy to see your updated repo.

Great work.

Just one feature request if its possible.

  1. Button to Start/Stop the VM's

@tomaae
Copy link
Owner

tomaae commented Mar 23, 2024

ah, I see. they are changing something again in new scale. will have to install it somewhere to see whats going on there.

you can start and stop VMs already, there is a service for it. You can create button from service if you need, just careful of missclick :)

@tomaae tomaae added the planned To be implemented in future release label Mar 23, 2024
@tomaae tomaae marked this pull request as draft March 23, 2024 15:09
@github-actions github-actions bot added the conflict Conflict with other software label Apr 3, 2024
@tomaae
Copy link
Owner

tomaae commented Apr 3, 2024

Most of these is now implemented, except swap.
I will leave this PR in draft for now, may want to add swap too.

@mrsaxenanitin
Copy link
Author

Most of these is now implemented, except swap. I will leave this PR in draft for now, may want to add swap too.
There are few things I wanted to highlight:
I am running
TrueNAS Version: Dragonfish-24.04-RC.1
We need to remove extra comma from the params to make it work:
if self._is_scale and self._version_major >= 23: tmp_params = { "graphs": [ {"name": "load"}, {"name": "cputemp"}, {"name": "cpu"}, {"name": "arcsize"}, {"name": "arcactualrate"}, {"name": "memory"} ], "reporting_query": { "start": "-90", "end": "-30", "aggregate": True } }
Rest everything seems to work perfectly

@mrsaxenanitin
Copy link
Author

Can you please add a toggle for the VM to Start/Stop?
This will help everyone to automate the VM from Home Assistant enhancing the functionality of your integration to multi-fold.

@tomaae
Copy link
Owner

tomaae commented Apr 4, 2024

Most of these is now implemented, except swap. I will leave this PR in draft for now, may want to add swap too.
There are few things I wanted to highlight:
I am running
TrueNAS Version: Dragonfish-24.04-RC.1
We need to remove extra comma from the params to make it work:
if self._is_scale and self._version_major >= 23: tmp_params = { "graphs": [ {"name": "load"}, {"name": "cputemp"}, {"name": "cpu"}, {"name": "arcsize"}, {"name": "arcactualrate"}, {"name": "memory"} ], "reporting_query": { "start": "-90", "end": "-30", "aggregate": True } }
Rest everything seems to work perfectly

what do you mean?

@tomaae
Copy link
Owner

tomaae commented Apr 4, 2024

Can you please add a toggle for the VM to Start/Stop? This will help everyone to automate the VM from Home Assistant enhancing the functionality of your integration to multi-fold.

Like I said, that is already possible.
image

@Deldion
Copy link

Deldion commented Apr 16, 2024

Glad to see you're back at it @tomaae

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict Conflict with other software planned To be implemented in future release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants