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.

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

Net 818 #3

Merged
merged 14 commits into from
Dec 3, 2024
Merged

Net 818 #3

merged 14 commits into from
Dec 3, 2024

Conversation

surprise30
Copy link
Owner

Added support to report amount of adjancies to every interface which is enabled for ISIS ( excluding passive)

@surprise30 surprise30 requested a review from taktv6 November 12, 2024 12:20
taktv6
taktv6 reviewed Nov 12, 2024
View reviewed changes
pkg/features/isis/collector.go Outdated
@@ -77,6 +83,12 @@ func (c *isisCollector) Collect(client collector.Client, ch chan<- prometheus.Me
}
}

var i interfaces
Copy link
Collaborator

Choose a reason for hiding this comment

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

i is to be used for an int loop invariant. Would suggest ifas as short form of interfaces.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done

pkg/features/isis/collector.go Outdated
}
labels := append(labelValues,
i.InterfaceName,
"",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this empty string?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed, created new lables for this metric

pkg/features/isis/collector.go Outdated
i.InterfaceLevelData.Level)
c, err := strconv.Atoi(i.InterfaceLevelData.AdjacencyCount)
if err != nil {
log.Errorf("unable to convert number of adjanceis: %q", i.InterfaceLevelData.AdjacencyCount)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really want to succeed after logging the error?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done

taktv6
taktv6 reviewed Nov 28, 2024
View reviewed changes
pkg/features/isis/rpc.go Outdated
IsisInterface []struct {
InterfaceName string `xml:"interface-name"`
InterfaceLevelData struct {
Level string `xml:"level"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this an int?

pkg/features/isis/rpc.go Outdated
InterfaceName string `xml:"interface-name"`
InterfaceLevelData struct {
Level string `xml:"level"`
AdjacencyCount string `xml:"adjacency-count"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

int?

pkg/features/isis/collector.go Outdated
adjState = prometheus.NewDesc(prefix+"adjacency_state", "The ISIS Adjacency state (0 = DOWN, 1 = UP, 2 = NEW, 3 = ONE-WAY, 4 =INITIALIZING , 5 = REJECTED)", l, nil)
lablesForInterfaceMetrics := []string{"target", "interface_name", "level"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename to interfaceMetricsLabels ?

pkg/features/isis/collector.go Outdated
upCount *prometheus.Desc
totalCount *prometheus.Desc
adjState *prometheus.Desc
upCount *prometheus.Desc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please be so kind and suffix them all with Desc.

pkg/features/isis/collector.go Outdated
@@ -3,26 +3,32 @@
package isis

import (
"github.com/pkg/errors"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Imports should be sorted:

  1. standard library
  2. project internal packages
  3. project external packages
  4. renamed packages (first internal, then external)

taktv6
taktv6 reviewed Dec 3, 2024
View reviewed changes
pkg/features/isis/collector.go Outdated
totalCountDesc = prometheus.NewDesc(prefix+"total_count", "Number of ISIS Adjacencies", l, nil)
l = append(l, "interface_name")
LSPIntervalDesc = prometheus.NewDesc(prefix+"lsp_interval", "The ISIS LSP interval", l, nil)
CNMPIntervalDesc = prometheus.NewDesc(prefix+"cnmp_interval", "The ISIS CNMP interval", l, nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

CSNP interval

taktv6
taktv6 requested changes Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@taktv6 taktv6 left a comment

Choose a reason for hiding this comment

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

LGTM in general. Just not happy with that one name.

pkg/features/isis/collector.go Outdated
}
}

func (*isisCollector) additionalIsIsInterfaceMetrics(ifas interfaces, ch chan<- prometheus.Metric, labelValues []string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method name is not really saying too precisely what it does. Either we group things so that we can find a common precise term or we merge this into isisInterfaces method.

Vincent Vilenchik added 2 commits December 3, 2024 14:46
taktv6
taktv6 approved these changes Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@taktv6 taktv6 left a comment

Choose a reason for hiding this comment

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

LGTM

@surprise30 surprise30 merged commit 3b5703a into main Dec 3, 2024
czerwonk pushed a commit that referenced this pull request Dec 12, 2024
@surprise30
Additional metrics for ISIS (czerwonk#266)
d272878 
* added a feature for macsec

* small refactoring of some functions

* added support for fec mode to interfaces plugin

* small edits

* small edits

* small edits + chamged the mtu metric name

* removed unnecessary string clean-up

* small fixes as per pull request

* tiny editing

* Net 818 (#3)

* adding number of isis adanceis per interface. Saving progress

* finished

* saving

* saving

* fixed some issues from pull request

* fixes from merge request

* added small fixes from pull review

* added four new metrics for isis

* added additional ISIS metrics

* sorted imports

* applied fixes from MR

* small edit

* small re-org

* small re-org

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>
surprise30 added a commit that referenced this pull request Feb 19, 2025
@surprise30 @czerwonk
Added arp metric (czerwonk#268)
ec5ab36 
* added a feature for macsec

* small refactoring of some functions

* added support for fec mode to interfaces plugin

* small edits

* small edits

* small edits + chamged the mtu metric name

* removed unnecessary string clean-up

* small fixes as per pull request

* tiny editing

* Net 818 (#3)

* adding number of isis adanceis per interface. Saving progress

* finished

* saving

* saving

* fixed some issues from pull request

* fixes from merge request

* added small fixes from pull review

* added four new metrics for isis

* added additional ISIS metrics

* sorted imports

* applied fixes from MR

* small edit

* small re-org

* small re-org

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>

* Add ARP feature  (#4)

* init config for arp feature

* small fix of type and added tests

* small changes in test file

* reduced the test xml

* small edit in test file

* small edits

* turned off isis

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>
Co-authored-by: Daniel Brendgen-Czerwonk <czerwonk@users.noreply.github.com>
surprise30 added a commit that referenced this pull request Feb 19, 2025
@surprise30 @czerwonk
Added arp metric (czerwonk#268)
90cddd3 
* added a feature for macsec

* small refactoring of some functions

* added support for fec mode to interfaces plugin

* small edits

* small edits

* small edits + chamged the mtu metric name

* removed unnecessary string clean-up

* small fixes as per pull request

* tiny editing

* Net 818 (#3)

* adding number of isis adanceis per interface. Saving progress

* finished

* saving

* saving

* fixed some issues from pull request

* fixes from merge request

* added small fixes from pull review

* added four new metrics for isis

* added additional ISIS metrics

* sorted imports

* applied fixes from MR

* small edit

* small re-org

* small re-org

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>

* Add ARP feature  (#4)

* init config for arp feature

* small fix of type and added tests

* small changes in test file

* reduced the test xml

* small edit in test file

* small edits

* turned off isis

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>

---------

Co-authored-by: Vincent Vilenchik <vincent.vilenchik@deepl.com>
Co-authored-by: Daniel Brendgen-Czerwonk <czerwonk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taktv6 taktv6 taktv6 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@surprise30 @taktv6