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.

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

feat: add metrics for count of known NNC and NCs #3389

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbtr
Copy link
Contributor

@rbtr rbtr commented Feb 4, 2025

Reason for Change:

Exposes a metric for known NNC and NCs in the NNC reconciler and sync host NC loop.

Issue Fixed:

Requirements:

Notes:

@rbtr rbtr requested a review from a team as a code owner February 4, 2025 22:10
@rbtr rbtr requested a review from csfmomo February 4, 2025 22:10
@rbtr rbtr force-pushed the feat/nnc-status-metric branch from dac4264 to 14e2a25 Compare February 4, 2025 22:10
@rbtr
Copy link
Contributor Author

rbtr commented Feb 5, 2025

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rbtr rbtr self-assigned this Feb 5, 2025
)
ncs = prometheus.NewGauge(
prometheus.GaugeOpts{
Name: "nnc_ncs",
Copy link
Member

Choose a reason for hiding this comment

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

Typically, don't prometheus metrics that are counts end with _total?

Copy link
Contributor Author

@rbtr rbtr Feb 6, 2025

Choose a reason for hiding this comment

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

Yes, but these are not counts in the way that Prometheus uses that term.

They say add _total

for a unit-less accumulating count

This is an instantaneous count and may go up or down, ie not accumulating. I don't think total is appropriate. You wouldn't d/dt (nnc_ncs) to get a rate, for example, but d/dt (http_requests_total) is reasonable and what they intend.

Their general guidance is to suffix the metric with the plural units - NC is the unit here, as is IP in the ones above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants