Skip to content

SNMP profiles units and description generation #20163

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
May 2, 2025
Merged

SNMP profiles units and description generation #20163

merged 20 commits into from
May 2, 2025

Conversation

Ancairon
Copy link
Member

Summary

After some trial and error with the prompt, I have the first batch of files ready.

@netdata/product and @ilyam8 I think merging these in this fashion will make easier to quickly have a glance and see if something needs manual editing, wdyt?

I will take a look myself on the units, and if I find something off I will add some commits.

@Ancairon Ancairon requested a review from shyamvalsan April 23, 2025 06:42
@Ancairon Ancairon self-assigned this Apr 23, 2025
@github-actions github-actions bot added area/collectors Everything related to data collection collectors/go.d area/go labels Apr 23, 2025
shyamvalsan
shyamvalsan previously approved these changes Apr 23, 2025
View reviewed changes
ilyam8
ilyam8 reviewed Apr 23, 2025
View reviewed changes
src/go/plugin/go.d/config/go.d/snmp.profiles/default/_aruba-switch-cpu-memory.yaml
- OID: 1.3.6.1.4.1.14823.2.2.1.2.1.15.1.3
# core check only
name: memory.used
scale_factor: 1000
description: used memory in kilobytes
unit: "kBy"
Copy link
Member

Choose a reason for hiding this comment

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

kBy, units it braces (e.g. {packet}). I don't think our UI supports (understands) auto-scaling for such units.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ralphm can you elaborate here? Do we prefer no auto-scaling for the units to favor the UCUM way of having them?

Copy link
Member

Choose a reason for hiding this comment

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

@Ancairon, I assumed that these units do not work. Based on the fact that I have never seen them 😄

Copy link
Member

Choose a reason for hiding this comment

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

@Ancairon, I assumed that these units do not work. Based on the fact that I have never seen them 😄

If they don't work, then this is a bug. We did work in the frontend as part of this effort: https://github.com/netdata/product/issues/3136.

Copy link
Member Author

Choose a reason for hiding this comment

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

So @ralphm in this case should it be kBy or By?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know where scale_factor comes from. Do we use it? I know that DataDog uses this to consume the value in bytes instead of the original kilobytes that is provided by the source. So if we do apply it, the unit should be By. If we don't kBy.

I am also wondering if kilobyte actually means 1000 bytes here, or really 1024 bytes. If the latter, the proper unit with prefix would be kiBy.

Copy link
Member

Choose a reason for hiding this comment

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

(but I think SI-prefixes are more common in SNMP)

Copy link
Member

Choose a reason for hiding this comment

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

I tested {packet} and I see that the scaling works. And we keep the braces... @ralphm is that expected?

Screenshot 2025-04-23 at 13 59 15

@Ancairon
Copy link
Member Author

I see a lot of inconsistencies.

I want to use resources from https://mibbrowser.online/mibdb_search.php?mib=TCP-MIB too, and provide the model with the mib, but that will be more complex.

PR will stay draft and I will work on it.

@Ancairon Ancairon changed the title _* SNMP profiles units and description generation SNMP profiles units and description generation Apr 30, 2025
@github-actions github-actions bot added the area/metadata Integrations metadata label May 2, 2025
@Ancairon Ancairon marked this pull request as ready for review May 2, 2025 06:11
@Ancairon
Copy link
Member Author

Ancairon commented May 2, 2025

@ilyam8 @netdata/product @ralphm

I think this is ready, updates yaml files with descriptions and units. I did print all distinct units and replaced illegal ones.

In another PR we will address the rest of the requirements we set for this. This should only touch the yaml files.

@ilyam8 ilyam8 merged commit 12f0ba9 into netdata:master May 2, 2025
106 checks passed
@ilyam8
Copy link
Member

ilyam8 commented May 2, 2025

@Ancairon There are 347 TBD units

$ git grep TBD | wc -l
347

What do we need to discuss to change them?

@Ancairon
Copy link
Member Author

Ancairon commented May 2, 2025

we need to review them, "to be determined"

@Ancairon
Copy link
Member Author

Ancairon commented May 2, 2025

they are all the units the model could not fill in

@ilyam8
Copy link
Member

ilyam8 commented May 2, 2025

I see many of them are very clear if you just read them. I see:

  • A group of metrics that are tags. They don't have units. E.g.:
tripplite-pdu.yaml-        description: The name of the manufacturer.
tripplite-pdu.yaml:        unit: "TBD"
--
tripplite-pdu.yaml-        description: The model designation.
tripplite-pdu.yaml:        unit: "TBD"
  • A group of status metrics. Unit should be "status". E.g.:
opengear-infrastructure-manager.yaml-        description: The status of the DCD signal
opengear-infrastructure-manager.yaml:        unit: "TBD"
--
opengear-infrastructure-manager.yaml-        description: The status of the DTR signal
opengear-infrastructure-manager.yaml:        unit: "TBD"
--
opengear-infrastructure-manager.yaml-        description: The status of the DSR signal
opengear-infrastructure-manager.yaml:        unit: "TBD"
--
opengear-infrastructure-manager.yaml-        description: The status of the CTS signal
opengear-infrastructure-manager.yaml:        unit: "TBD"
--
opengear-infrastructure-manager.yaml-        description: The status of the RTS signal
opengear-infrastructure-manager.yaml:        unit: "TBD"

@Ancairon
Copy link
Member Author

Ancairon commented May 2, 2025

when I review them I guess these will be the easy ones to change, for now I think it is okay to move to the next step

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ralphm ralphm ralphm left review comments

@ilyam8 ilyam8 ilyam8 approved these changes

@shyamvalsan shyamvalsan shyamvalsan left review comments

Assignees

@Ancairon Ancairon

Labels
area/collectors Everything related to data collection area/go area/metadata Integrations metadata collectors/go.d
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ancairon @ilyam8 @ralphm @shyamvalsan