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

Junos dwdm interface sensor support #7714

Merged
merged 16 commits into from Dec 13, 2017

Conversation

Projects
None yet
4 participants
@bergroth
Contributor

bergroth commented Nov 13, 2017

four new sensors
Chromatic Dispersion ps/nm
Delay s (seconds)
Q_factor (dB)
preFEC_BER (ratio)

reuse of
SNR, dbm, temperature, current

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Junos dwdm interface sensor support …
  four new sensors
    Chromatic Dispersion ps/nm
    Delay s (seconds)
    Q_factor (dB)
    preFEC_BER (ratio)

  reuse of
    SNR, dbm, temperature, current
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 14, 2017

Member

Thanks.
It seems like the sensors need to be a little more generic.
q-factor -> quality_factor or just quality
prefec_ber -> ber (for junos, you can just set the description to PreFEC BER)

Some of your headers need fixing.
It seems like you could probably use yaml for the sensor discovery in most cases here.

Member

murrant commented Nov 14, 2017

Thanks.
It seems like the sensors need to be a little more generic.
q-factor -> quality_factor or just quality
prefec_ber -> ber (for junos, you can just set the description to PreFEC BER)

Some of your headers need fixing.
It seems like you could probably use yaml for the sensor discovery in most cases here.

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Nov 14, 2017

Contributor
Contributor

bergroth commented Nov 14, 2017

bergroth added some commits Nov 15, 2017

@laf

Some queries around the measurement values for the new sensors.

I think we can convert all of this to yaml with a little extra code. Would you be able to provide us with snmpwalk output for this device so we can simulate it and do the conversion?

@@ -25,6 +25,10 @@ Currently we have support for the following health metrics along with the values
| state | # |
| temperature | C |
| voltage | V |
| delay | s |
| quality_factor | dB |

This comment has been minimized.

@laf

laf Nov 15, 2017

Member

Any links to show that dB is the measurement of quality factor?

@laf

laf Nov 15, 2017

Member

Any links to show that dB is the measurement of quality factor?

@@ -25,6 +25,10 @@ Currently we have support for the following health metrics along with the values
| state | # |
| temperature | C |
| voltage | V |
| delay | s |
| quality_factor | dB |
| chromatic_disperision | ps/nm |

This comment has been minimized.

@laf

laf Nov 15, 2017

Member

Should this be ps/nm km?

@laf

laf Nov 15, 2017

Member

Should this be ps/nm km?

@@ -25,6 +25,10 @@ Currently we have support for the following health metrics along with the values
| state | # |
| temperature | C |
| voltage | V |
| delay | s |

This comment has been minimized.

@laf

laf Nov 15, 2017

Member

Still not sure if ms is a better value to store

@laf

laf Nov 15, 2017

Member

Still not sure if ms is a better value to store

'delay' => 'clock-o',
'chromatic_dispersion' => 'indent',
'ber' => 'sort-amount-desc',
'quality_factor' => 'arrows',

This comment has been minimized.

@laf

laf Nov 15, 2017

Member

area-chart maybe?

@laf

laf Nov 15, 2017

Member

area-chart maybe?

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Nov 15, 2017

Contributor
Contributor

bergroth commented Nov 15, 2017

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Nov 15, 2017

Contributor
Contributor

bergroth commented Nov 15, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 15, 2017

Member
  1. To set description to IF-MIB::ifDescr.index 2. to multiply two values to get BER: exponent and mantisa 3. the index for max an min has a different index compare to the interface index.
  1. We can make that available by caching the data.

  2. If you put that work into a function we can add support for passing a function in yaml.

  3. This could be trickier, with the data we can see.

juniper.net/documentation/en_US/junos/topics/concept/otn-signal-degrade-monitoring-understanding.html http://mapyourtech.com/entries/general/what-is-q-factor-and-what-is-its-importance-

Juniper link seems to suggest dBQ?

I would be nice to have ms, us, ns and ps.

Yeah we can't do that :(

Member

laf commented Nov 15, 2017

  1. To set description to IF-MIB::ifDescr.index 2. to multiply two values to get BER: exponent and mantisa 3. the index for max an min has a different index compare to the interface index.
  1. We can make that available by caching the data.

  2. If you put that work into a function we can add support for passing a function in yaml.

  3. This could be trickier, with the data we can see.

juniper.net/documentation/en_US/junos/topics/concept/otn-signal-degrade-monitoring-understanding.html http://mapyourtech.com/entries/general/what-is-q-factor-and-what-is-its-importance-

Juniper link seems to suggest dBQ?

I would be nice to have ms, us, ns and ps.

Yeah we can't do that :(

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 19, 2017

Member

@bergroth we need the snmpwalk in this format: snmpbulkwalk -OUneb -v2c -c COMMUNITY HOSTNAME .

Member

laf commented Nov 19, 2017

@bergroth we need the snmpwalk in this format: snmpbulkwalk -OUneb -v2c -c COMMUNITY HOSTNAME .

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Nov 20, 2017

Contributor
  • Updated with -OUneb https://pastebin.com/Ac8c0Fke

  • Q-factor according to the MIB are in unit dB
    jnxPMCurQ OBJECT-TYPE
    SYNTAX Integer32
    UNITS "0.1 dB"
    MAX-ACCESS read-only
    STATUS current
    DESCRIPTION
    "'Q' factor estimated at Rx Transceiver port "
    ::= { jnxOpticsPMCurrentEntry 5 }

and Cisco says the same
https://www.cisco.com/c/en/us/td/docs/optical/metroplanner/metroplanner_9_2_1/operations/guide/454mp921_opsguide/454mp921_interfacemodel.html

  • Librenms handles the IS units nicely. Stored as seconds and displayed by RRD and Librenms as ps.

screen shot 2017-11-20 at 07 42 15

Contributor

bergroth commented Nov 20, 2017

  • Updated with -OUneb https://pastebin.com/Ac8c0Fke

  • Q-factor according to the MIB are in unit dB
    jnxPMCurQ OBJECT-TYPE
    SYNTAX Integer32
    UNITS "0.1 dB"
    MAX-ACCESS read-only
    STATUS current
    DESCRIPTION
    "'Q' factor estimated at Rx Transceiver port "
    ::= { jnxOpticsPMCurrentEntry 5 }

and Cisco says the same
https://www.cisco.com/c/en/us/td/docs/optical/metroplanner/metroplanner_9_2_1/operations/guide/454mp921_opsguide/454mp921_interfacemodel.html

  • Librenms handles the IS units nicely. Stored as seconds and displayed by RRD and Librenms as ps.

screen shot 2017-11-20 at 07 42 15

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 20, 2017

Member

Thanks but can you provide the output as is from snmpbulkwalk -OUneb -v2c -c COMMUNITY HOSTNAME . - notice the trailing dot. Don't use any other OIDs. We need things like the interface data to match all of this up.

Member

laf commented Nov 20, 2017

Thanks but can you provide the output as is from snmpbulkwalk -OUneb -v2c -c COMMUNITY HOSTNAME . - notice the trailing dot. Don't use any other OIDs. We need things like the interface data to match all of this up.

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Nov 20, 2017

Contributor
Contributor

bergroth commented Nov 20, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 21, 2017

Member

IF-MIB and Sensors might be ok. I need the data to replicate this PR basically.

I've got base junos data so that should be ok.

Member

laf commented Nov 21, 2017

IF-MIB and Sensors might be ok. I need the data to replicate this PR basically.

I've got base junos data so that should be ok.

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Nov 22, 2017

Contributor

Included the if-mib as well

Contributor

bergroth commented Nov 22, 2017

Included the if-mib as well

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 22, 2017

Member

Got a link to the data?

Member

laf commented Nov 22, 2017

Got a link to the data?

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Nov 22, 2017

Contributor
Contributor

bergroth commented Nov 22, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 23, 2017

Member

Can you provide the data as one output in the format required. We don't need the data in both formats and split out like that,

Member

laf commented Nov 23, 2017

Can you provide the data as one output in the format required. We don't need the data in both formats and split out like that,

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 23, 2017

Member

This won't work without some back end changes I've made but this yaml for chromatic disperson works as expected:

mib: JUNIPER-IFOPTICS-MIB:JNX-OPT-IF-EXT-MIB:IF-MIB
modules:
    sensors:
        pre-cache:
            data:
                - oid:
                    - ifDescr
        chromatic_dispersion:
            data:
                -
                    oid: jnxOpticsPMCurrentTable
                    value: jnxPMCurChromaticDispersion
                    num_oid: .1.3.6.1.4.1.2636.3.71.1.2.1.1.1.
                    entPhysicalIndex: '{{ $index }}'
                    entPhysicalIndex_measured: 'ports'
                    descr: '{{ $ifDescr }} CD'
                    index: 'jnxPMCurChromaticDispersion.{{ $index }}'

I'll continue to move all sensors over to this format. If you can rebase this PR against master and then I'll provide you the yaml files and patches to the core code, hopefully in a few days.

Member

laf commented Nov 23, 2017

This won't work without some back end changes I've made but this yaml for chromatic disperson works as expected:

mib: JUNIPER-IFOPTICS-MIB:JNX-OPT-IF-EXT-MIB:IF-MIB
modules:
    sensors:
        pre-cache:
            data:
                - oid:
                    - ifDescr
        chromatic_dispersion:
            data:
                -
                    oid: jnxOpticsPMCurrentTable
                    value: jnxPMCurChromaticDispersion
                    num_oid: .1.3.6.1.4.1.2636.3.71.1.2.1.1.1.
                    entPhysicalIndex: '{{ $index }}'
                    entPhysicalIndex_measured: 'ports'
                    descr: '{{ $ifDescr }} CD'
                    index: 'jnxPMCurChromaticDispersion.{{ $index }}'

I'll continue to move all sensors over to this format. If you can rebase this PR against master and then I'll provide you the yaml files and patches to the core code, hopefully in a few days.

@laf laf added this to the 1.35 milestone Nov 27, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 6, 2017

Member

@bergroth how you getting on with this?

Member

laf commented Dec 6, 2017

@bergroth how you getting on with this?

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Dec 6, 2017

Contributor
Contributor

bergroth commented Dec 6, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 6, 2017

Member

Have to keep ber in php for now then.

Member

laf commented Dec 6, 2017

Have to keep ber in php for now then.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Dec 8, 2017

Member

@bergroth yeah, keep BER in php for now.

FYI, the wireless sensor code supports multiple oids and "aggregating" them. But that code is very different from the sensor code, for now.

Member

murrant commented Dec 8, 2017

@bergroth yeah, keep BER in php for now.

FYI, the wireless sensor code supports multiple oids and "aggregating" them. But that code is very different from the sensor code, for now.

@laf

So nearly ok for me with two exceptions:

  1. You can't edit build.sql so please revert that chage.

  2. You've changed sensor_divisor to allow null and remove the default value, please set allow null to false and the default divisor to 1.

After those, I'm fine with this being merged.

@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Dec 10, 2017

Contributor
Contributor

bergroth commented Dec 10, 2017

Show outdated Hide outdated build.sql
@bergroth

This comment has been minimized.

Show comment
Hide comment
@bergroth

bergroth Dec 11, 2017

Contributor
Contributor

bergroth commented Dec 11, 2017

Show outdated Hide outdated includes/discovery/sensors/current/junos.inc.php
Show outdated Hide outdated sql-schema/223.sql
@laf

laf approved these changes Dec 11, 2017

I've resolved the last few issues. All good from my perspective now.

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Dec 11, 2017

The inspection completed: 1 updated code elements

scrutinizer-notifier commented Dec 11, 2017

The inspection completed: 1 updated code elements

@murrant murrant merged commit 84fdfdf into librenms:master Dec 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

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