-
Notifications
You must be signed in to change notification settings - Fork 20
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
non-leader metrics-prometheus-collector eagerly computes metrics #42
Conversation
pwittrock
commented
Mar 31, 2023
- sampler pushes metrics to all collector replicas
- non-leader collector computes and calculates metrics, but doesn't publish
4114c28
to
f86192b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
c7f834b
to
f76c72d
Compare
ecf0bf5
to
0917237
Compare
ebe9d62
to
d958175
Compare
/triage accepted |
if options.LeaderOptions.LeaderElection { | ||
metrics.Col.IsLeaderElected.Store(false) | ||
} else { | ||
metrics.Col.IsLeaderElected.Store(true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if options.LeaderOptions.LeaderElection { | |
metrics.Col.IsLeaderElected.Store(false) | |
} else { | |
metrics.Col.IsLeaderElected.Store(true) | |
} | |
metrics.Col.IsLeaderElected.Store(!options.LeaderOptions.LeaderElection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
00854f1
to
cbffb8b
Compare
Had to significantly re-work this after thoroughly testing it in a cluster. We should follow up with some e2e tests that run for ~hours and do things like rolling updates to ensure a proper handoff. That is a non-trivial task though, and shouldn't block fixing the issue. |
ms.doLeaderElection(ctx) | ||
} else { | ||
electedMetric.WithLabelValues(os.Getenv("POD_NAME")).Set(1) | ||
ms.Col.IsLeaderElected.Store(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems redundant, since we already set this to !options.LeaderElection
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved with nits
* sampler pushes metrics to all collector replicas * samplers query DNS and headless service at startup and periodically -- fix periodic registration delay from collectors * collectors explicitly register themselves at startup and periodically -- fix DNS propagation delay * sampler sets pod name, namespace and container name from container labels when using containerd * non-leader collector computes and calculates metrics, but doesn't publish to prometheus
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ndipebot, pwittrock The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |