Skip to content

SNMP first cisco yaml file pass#20246

Merged
Ancairon merged 10 commits intonetdata:masterfrom
Ancairon:custom-units-descriptions
May 12, 2025
Merged

SNMP first cisco yaml file pass#20246
Ancairon merged 10 commits intonetdata:masterfrom
Ancairon:custom-units-descriptions

Conversation

@Ancairon
Copy link
Copy Markdown
Member

@Ancairon Ancairon commented May 7, 2025

Summary

edited my first cisco yaml, let's review and I can proceed with more yamls, either in this PR or on later PRs

also removed a non-used yaml file, referenced by nothing

@Ancairon Ancairon self-assigned this May 7, 2025
@Ancairon Ancairon requested a review from ilyam8 as a code owner May 7, 2025 09:43
@github-actions github-actions bot added area/collectors Everything related to data collection collectors/go.d area/go labels May 7, 2025
@sashwathn
Copy link
Copy Markdown
Contributor

@Ancairon : I think we do use {status} and other such units for count. @ralphm can you confirm?
The problem is if it is easily understood for a user if it says status 4 for example.

@Ancairon
Copy link
Copy Markdown
Member Author

Ancairon commented May 8, 2025

these have to be UCUM units, {status} can also be {potato}, here the status is probably a number, and it does not have a unit, we are not talking about number of statuses, but a single status, so that's why I put {1}

@ilyam8
Copy link
Copy Markdown
Member

ilyam8 commented May 8, 2025

these have to be UCUM units

Why? A lot of Netdata charts have status/state unit. I don't think using 1 will be helpful for anybody.

description: "The most recent measurement seen by the sensor"
unit: "TBD"
unit: "1"
family: "Switches/Sensors"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sashwathn @ralphm

Did you suggest using device type as the first element of the family? If so, why do we need it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ilyam8 : Ideally I could envision a ToC structure like this:

SNMP --> Vendor --> Device Type --> Specific Families (Global, specific metrics etc).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where do you put this structure? Network devices are virtual nodes. It is like adding "Server" root section for all server metrics (Compute, etc).

Copy link
Copy Markdown
Contributor

@sashwathn sashwathn May 8, 2025

Choose a reason for hiding this comment

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

@ilyam8 : This is what we have currently and instead of Server we call it System..

What I would expect is a hierarchy like this:

  • SNMP
    • Global / Common
      • All Common metrics on all SNMP devices
    • Vendor A
      • Global / Common
      • Switches
        • Metrics only specific to Vendor A Switches
      • Firewall
        • Metrics only specific to Vendor A Firewalls
      • Access Points
        • Metrics only specific to Vendor A Access Points
    • Vendor B
      • Global / Common
      • Switches
        • Metrics only specific to Vendor B Switches
      • Firewall
        • Metrics only specific to Vendor B Firewalls
      • Access Points
        • Metrics only specific to Vendor B Access Points

I don't know what restrictions we may have in here but from a UX perspective , I think this is what our users expect.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sashwathn but you will have this for single nodes view too... Is that ok?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ilyam8 : It doesn't help when you put up valid points.. ;)
Let me think about this a little bit.

Copy link
Copy Markdown
Member

@ralphm ralphm left a comment

Choose a reason for hiding this comment

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

I'm fine with the family-as-section values for now. Given the similarity of some metrics to generic metrics we use for "regular" boxes, we want to eventually map these to the same contexts.

Other comments inline.

name: entSensorValue
description: "The most recent measurement seen by the sensor"
unit: "TBD"
unit: "1"
Copy link
Copy Markdown
Member

@ralphm ralphm May 8, 2025

Choose a reason for hiding this comment

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

This particular MIB is problematic as it is meant to convey a sequence of generic "sensors" as a EntSensorValueEntry:

EntSensorValueEntry ::= SEQUENCE {
        entSensorType            SensorDataType,
        entSensorScale           SensorDataScale,
        entSensorPrecision       SensorPrecision,
        entSensorValue           SensorValue,
        entSensorStatus          SensorStatus,
        entSensorValueTimeStamp  TimeStamp,
        entSensorValueUpdateRate SensorValueUpdateRate,
        entSensorMeasuredEntity  EntPhysicalIndexOrZero
}

The unit of the value in entSensorValue depends on entSensorType below, which can have a bunch of different value strings that each need to map to a proper UCUM unit. It then uses entSensorScale, an enum of SI prefixes that need to applied to the unit, too.

Reading the MIB file directly should give a pretty good picture.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

so what should we do in the profile? I understand that maybe to collector will need to understand that this is a sequence that has a variable unit (not straightforward) and possibly add it on the fly.

@ralphm
Copy link
Copy Markdown
Member

ralphm commented May 8, 2025

these have to be UCUM units

Why? A lot of Netdata charts have status/state unit. I don't think using 1 will be helpful for anybody.

This work also serves as a catalyst to using UCUM for expressing units everywhere, paving the way to proper OpenTelemetry compatibility.

@ralphm
Copy link
Copy Markdown
Member

ralphm commented May 8, 2025

@Ancairon : I think we do use {status} and other such units for count. @ralphm can you confirm? The problem is if it is easily understood for a user if it says status 4 for example.

Yes, we probably need to special case annotations that are not things-to-counted. So the UI can have "4 apples" for {apple} with the value 4, and maybe just "4" for {status}.

Ideally we'd also support (and interpret) enums, in Netdata in general, and for interpreting SNMP values specifically (as MIB files have enums for many things), but that is a separate piece of work, I think.

@ralphm
Copy link
Copy Markdown
Member

ralphm commented May 8, 2025

As a follow-up, this is also still being discussed in the OpenTelemetry and Prometheus worlds: open-telemetry/opentelemetry-specification#1711.

@ilyam8
Copy link
Copy Markdown
Member

ilyam8 commented May 8, 2025

and maybe just "4" for {status}.

To track status, we should create a separate dimension for each status (0 - inactive, 1 - active). Without this separation, the status metric becomes meaningless when aggregated over time or across multiple nodes.

@Ancairon
Copy link
Copy Markdown
Member Author

Ancairon commented May 9, 2025

To track status, we should create a separate dimension for each status (0 - inactive, 1 - active). Without this separation, the status metric becomes meaningless when aggregated over time or across multiple nodes.

I will make this a note. Is it fine to use "{status}" as a special unit that has this treatment in the collector?

Latest commit adds an annotated unit @ralphm that we will try and handle in the collector.

@Ancairon Ancairon requested review from ilyam8, ralphm and sashwathn May 9, 2025 06:22
sashwathn
sashwathn previously approved these changes May 9, 2025
@ralphm
Copy link
Copy Markdown
Member

ralphm commented May 9, 2025

The unit {status} is fine, but I think any special handling by the collector should be explicit, not as a magic value. So if each value needs is own dimension, we should have some other property to trigger this.

The same goes for the special handling of generic sensors, where the unit and scale are defined in a different OID. Until we have that, we don't know the unit, so 1 is appropriate. I'm not sure if it should be explicit or that leaving off a unit field implies 1. But not the magic annotation in the last commit.

@Ancairon
Copy link
Copy Markdown
Member Author

Ancairon commented May 9, 2025

@ralphm is unit {1} supported on the UI? Using it and showing 1 for unit will be super weird, almost broken-looking

@ralphm
Copy link
Copy Markdown
Member

ralphm commented May 9, 2025

No, you need to use 1, {1}is silly.

@Ancairon
Copy link
Copy Markdown
Member Author

Ancairon commented May 9, 2025

No, you need to use 1, {1}is silly.

I will fix, but I am not talking about the format, but the actual rendering in the UI.

@Ancairon Ancairon force-pushed the custom-units-descriptions branch from 426170c to 90fe061 Compare May 9, 2025 11:23
@ralphm
Copy link
Copy Markdown
Member

ralphm commented May 9, 2025

The current list of supported units is here, with metadata on how they need be shown. E.g. the unit 1 has this:

    1: {
      symbol: "1",
      name: "",
      print_symbol: "",
      is_metric: false,
      is_special: false,
      is_scalable: true,
      is_binary: false,
    },

This means that it does not use any symbol after the number, and that it does not allow metric (SI) prefixes, but does allow scaling.

@Ancairon
Copy link
Copy Markdown
Member Author

@ralphm you are your requested changes relevant anymore? Can we merge or is there something more needed?

Copy link
Copy Markdown
Member

@ralphm ralphm left a comment

Choose a reason for hiding this comment

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

Let's merge this for now and then iterate.

@Ancairon Ancairon merged commit 2081315 into netdata:master May 12, 2025
106 checks passed
@Ancairon Ancairon deleted the custom-units-descriptions branch May 12, 2025 11:44
@Ancairon
Copy link
Copy Markdown
Member Author

Ancairon commented Jun 2, 2025

related to #20390

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

Labels

area/collectors Everything related to data collection area/go collectors/go.d

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants