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

Add Proxmox integration (ECOINT-50) #2555

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

aureleoules
Copy link
Member

@aureleoules aureleoules commented Dec 6, 2024

What does this PR do?

This pull is an hackathon project.
It adds an agent integration for Proxmox with an integration test and a dashboard.

image

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If this PR includes a log pipeline, please add a description describing the remappers and processors.

Additional Notes

Anything else we should know when reviewing?

@aureleoules aureleoules requested review from a team as code owners December 6, 2024 09:18
@aureleoules aureleoules marked this pull request as draft December 6, 2024 09:18
@aureleoules aureleoules force-pushed the aurele.oules/proxmox-integration branch from b9a2904 to efc4cec Compare December 6, 2024 09:51
@eho1307 eho1307 requested review from a team and JoshPatel13 and removed request for a team December 11, 2024 19:39
@aureleoules aureleoules marked this pull request as ready for review December 12, 2024 09:06
Copy link
Contributor

@eho1307 eho1307 left a comment

Choose a reason for hiding this comment

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

Hey @aureleoules helping push this along since there are customer requests for it. Thanks for building an integration during the hackathon! I've added comments to ensure it passes all our internal requirements. Let me know if you have any questions or concerns.

proxmox/CHANGELOG.md Outdated Show resolved Hide resolved
proxmox/README.md Outdated Show resolved Hide resolved
proxmox/README.md Show resolved Hide resolved
proxmox/README.md Outdated Show resolved Hide resolved
proxmox/README.md Outdated Show resolved Hide resolved
proxmox/manifest.json Outdated Show resolved Hide resolved
"changelog": "CHANGELOG.md",
"description": "Monitor Proxmox with Datadog",
"title": "Proxmox",
"media": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

You can reference the image of the dashboard you end up uploading

},
"metrics": {
"prefix": "proxmox.",
"check": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a metric that is always submitted and represents that the integration is working? If so, please specify that here

proxmox/manifest.json Outdated Show resolved Hide resolved
proxmox/metadata.csv Show resolved Hide resolved
@aliciascott aliciascott added the editorial review Waiting on a more in-depth review from a docs team editor label Dec 13, 2024
@aliciascott
Copy link
Contributor

Thank you! I've created an editorial card for our team to review further.

@dd-dominic dd-dominic changed the title Add Proxmox integration Add Proxmox integration (ECOINT-50) Dec 16, 2024
Copy link
Contributor

@iliakur iliakur left a comment

Choose a reason for hiding this comment

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

a couple small comments.

Not sure yet why the e2e tests are failing.

@aureleoules do you plan to add unit tests?

proxmox/datadog_checks/proxmox/__about__.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not submit a service check. We're slowly and gently deprecating those.


def check(self, _):
try:
headers = {"Authorization": f"PVEAPIToken={self.token_id}={self.token_secret}"}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if token_id and token_secret are None?

Copy link
Member Author

Choose a reason for hiding this comment

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

there is a check in the init function for this

Copy link
Contributor

Choose a reason for hiding this comment

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

@eho1307 do we need this for this integration?

}
},
"author": {
"support_email": "[email protected]",
Copy link
Contributor

Choose a reason for hiding this comment

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

@aureleoules are we, Datadog, officially going to support this?

proxmox.cpu_current,gauge,60,percent,,Current CPU usage,1,proxmox,cpu_current,,host:proxmox
proxmox.cpu_iowait,gauge,60,percent,,CPU iowait,1,proxmox,cpu_iowait,,host:proxmox
proxmox.cpu_max,gauge,60,percent,,Max CPU usage,1,proxmox,cpu_max,,host:proxmox
proxmox.disk_read,gauge,60,bytes,,Disk read,1,proxmox,disk_read,,host:proxmox
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
proxmox.disk_read,gauge,60,bytes,,Disk read,1,proxmox,disk_read,,host:proxmox
proxmox.disk_read,gauge,60,byte,,Disk read,1,proxmox,disk_read,,host:proxmox

Here and elsewhere. Also seconds -> second

proxmox/pyproject.toml Outdated Show resolved Hide resolved
proxmox/tests/test_e2e.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hestonhoffman hestonhoffman left a comment

Choose a reason for hiding this comment

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

Left you some suggestions

To install the Proxmox integration, run the following Agent installation command and the steps below. For more information, see the [Integration Management](https://docs.datadoghq.com/agent/guide/integration-management/?tab=linux#install) documentation.

```shell
datadog-agent integration install datadog-vsphere==3.6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Vsphere correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch it should be datadog-proxmox

proxmox/README.md Outdated Show resolved Hide resolved
proxmox/assets/dashboards/proxmox_overview.json Outdated Show resolved Hide resolved
"id": 6462887281714400,
"definition": {
"type": "note",
"content": "**Proxmox**\n\nThis dashboard provides an overview of your Proxmox and the VMs running. ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"content": "**Proxmox**\n\nThis dashboard provides an overview of your Proxmox and the VMs running. ",
"content": "**Proxmox**\n\nThis dashboard provides an overview of your Proxmox and VMs.",

aureleoules and others added 3 commits January 16, 2025 11:05
Co-authored-by: Heston Hoffman <[email protected]>
Co-authored-by: Heston Hoffman <[email protected]>
Co-authored-by: Ilia Kurenkov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial review Waiting on a more in-depth review from a docs team editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants