Update Portworx Integration: metric names and readme#2974
Conversation
6dbbaa0 to
607cbfa
Compare
|
Hi @steveny91 @rtrieu I need to update the portworx contact person email and info as well in the code maintainers. |
|
Hi @KritikaG05, we have an on-call person who should be handling or triaging this PR today. |
domalessi
left a comment
There was a problem hiding this comment.
Thanks for the update! The new Kubernetes-based setup flow is clearer than the old host-based instructions. A few structural items to address before merging:
Missing required sections:
- Service Checks subsection under Data Collected is missing. The manifest references a
service_checks.jsonasset, so the README should either list the service checks or state "The Portworx integration does not include any service checks." - Support section is missing.
manifest.jsonreferencesREADME.md#Support, but no## Supportheading exists. Forintegrations-extras, this section should direct users to your support team (Pure Storage / Portworx).
Step numbering structure is confusing:
- Steps 1, 2.1, 2.2 sit under
### Installation, but Step 2.3 jumps over to### Configuration. Then Step 3 follows under Configuration. Either keep all numbered steps under one heading, or restart numbering when crossing the Installation/Configuration boundary. - The
#### Step N - Titleheading style with hyphen separator is atypical. Consider dropping the "Step N -" prefix and using descriptive sentence-case headings (e.g.,#### Create the Datadog credentials Secret).
metadata.csv regressions:
- Many new metrics with byte/second values are missing
unit_name(e.g.,px_backup_stats_backup_size,px_backup_stats_backup_duration_seconds,px_device_delete_discard_bytes_total,px_pool_stats_provisioned_bytes,px_volume_usage_bytes, etc.). Please populateunit_name(andper_unit_namewhere applicable) for all metrics whose descriptions mention bytes, seconds, or other units. - The previous metric set had
short_namepopulated for every metric, but every new metric leavesshort_nameempty. This is a regression —short_nameis used in the Datadog UI. Please populate it.
Pre-existing nits in nearby unchanged lines (optional, since you're already touching the Overview):
- L7: "your Portworx Cluster" — "Cluster" should be lowercase.
- L8: missing Oxford comma — "disk usage, latency, and throughput".
|
Hi @domalessi |
Co-authored-by: domalessi <111786334+domalessi@users.noreply.github.com>
Co-authored-by: domalessi <111786334+domalessi@users.noreply.github.com>
domalessi
left a comment
There was a problem hiding this comment.
Thanks for the changes, @KritikaG05 ! Approved from an editorial perspective.
|
Thanks @domalessi for the approval. @rtrieu Could you please guide me with the next steps to merge this PR? I do not have an option to merge. |
What does this PR do?
This PR updates the Portworx integration README and metrics to align with the latest Portworx documentation and current integration behavior.
The changes primarily include documentation updates and metadata adjustments, with no functional code changes.
Motivation
What inspired you to submit this pull request?
The existing README and metric metadata were slightly outdated compared to the latest Portworx documentation.
This update ensures users have accurate information about available metrics and configuration, improving clarity and reducing confusion when using the Portworx integration.
Review checklist
no-changeloglabel attachedAdditional Notes
Anything else we should know when reviewing?
This PR only updates documentation and metadata. No changes to functionality, integration behavior, or metric collection logic are included.