Preliminary support for Proxmox VE guest monitoring

You are only considering the overall cost on the Observium side. If, for example, MySQL is running on another host, cost of forking rrdtool and database load are two completely separate things. While I totally agree about impact of fork()ing, your way still creates unnecessary load on the database server itself, which is even worse when both Observium and MySQL share the same limited pool of IO/s on a resource constrained (virtual) machine. With many concurrent Observium sessions it can sum up to quite a bit, especially if you are monitoring hundreds of devices and hit the moment when both discovery and polling processes run in the background. Even worse if you are using the syslog feature as well (should be moved from MySQL to ElasticSearch, or lean on Graylog2/Logstash, btw).

I also agree that Observium is not CPU bound, but it is IO bound, which is a lot worse. I do not agree, however, that relaxing a few simple rules makes code unmaintainable. I'm the first person to impose rules and procedures on others and I expect my employees to comply at all times, but rules aren't written in stone and can evolve over time to better fit new features, new requirements.

But, hey ... It's your project. I sincerely just wanted to help.

We don't accept code which doesn't conform to our standard conventions for doing things because it unnecessarily decreases the maintainability of the project.

Contributing code to an open source project is like contributing a baby to an orphanage, the future maintenance and updating of the code usually takes more time than it took to write the code in the first place.

We have standard ways of doing things because it makes it quick and easy for us to maintain the code, and for people to debug problems.

You're also gravely mistaken on the performance hit of MySQL queries, particularly this kind of query, and particularly in the context of graph.php.

The whole purpose of graph.php is to fork rrdtool, load one or more RRDs and build a graph, all of which costs at least two orders of magnitude more than the single MySQL lookup to fetch the entity data from the database.

Maintaining a large body of code which is not generally CPU performance bound almost always veers in favour of writing maintainable code rather than sacrificing maintainability in favour of performance.

Marko - i found the patch useful, i also the +1 for keeping queries down

now i just need to find a way to continue to make sure the patch holds with updates until adam releases it

although judging by the removal of patches this doesnt seem likely?

Well, then ... good luck with your coding style. I have donated the code. It's yours. Use it as is, change it, toss it ... makes no difference to me. I am writting extensions to Observium for my own purposes. In the process, I am more than willing to share, but I have no intention of becoming full time Observium developer, nor will I follow clearly flawed logic. People go to great lengths to reduce the expensive database access - you indirectly force developers to use it, even when it's not neccessary. Have you ever consideded your project might be used in companies with tens or even hundreds of engineers/support agents accessing Observium simultaneosly ? How many unnecessary queries are made as a result of your rules ?

Anyway ... I'll keep offering my patches, you'll keep rejecting them. But, perhaps, somewhere in between, someone on the list might find them useful and benefit from them even if they don't comply with your coding style.

You're probably missing the part where I won't commit anything that doesn't follow our coding style.

I'm passing all that to avoid accessing the database again. I already have to access the database and iterate over all discovered guests when im generating graph rows. Why not simply pass and re-use that data instead of making additional queries to generate RRD file paths, graph titles, etc ? With thousands of guests in the cluster that would make thousands of SELECTs while page is loading. Or am I missing something painfully obvious ?

+ $graph_array['device'] = $device['device_id'];
+ $graph_array['group'] = $group;
+ // - our custom parameters that will be passed to graph.php
+ // and subsequently to our graph generating code when
+ // invoked from <img src="graph.php? ..."/> tag
+ $graph_array['clustername'] = $guest['cluster_name'];
+ $graph_array['guestid'] = $guest['guest_id'];
+ $graph_array['guestname'] = $guest['guest_name'];
+ $graph_array['guesttype'] = $guest['guest_type'];
+ // Generate <img src="graph.php? ..."/> tags on current page

Why are you passing all of this?

Please follow the existing coding style and pass the minimum required to identify the entity. The Observium standard is to pass a database-specific id, with an option to find the id if entity-specific data is passed (see the ports graphs, generally we pass a port_id, but they can also be accessed via ifName or ifIndex)

Thanks a bunch! I'll keep an eye on other things I might have missed, ofc.

Marko just confirming your patch still works for standalone proxmox installs

I'm generally a Perl coder. PHP is not really my thing, but I'm getting used to it. In Perl, that would make a sound input params check, plus I generally like to be extra careful with input parameters. If you think it is unnecessary, I will remove it.