-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(ingest-limits): Add instance ring for service discovery #15819
Conversation
💻 Deploy preview deleted. |
a952d82
to
a13bae0
Compare
[join_after: <duration> | default = 0s] | ||
|
||
# Minimum duration to wait after the internal readiness checks have passed | ||
# but before succeeding the readiness endpoint. This is used to slowdown |
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.
# but before succeeding the readiness endpoint. This is used to slowdown | |
# but before succeeding the readiness endpoint. This is used to slow down |
# CLI flag: -ingest-limits.lifecycler.addr | ||
[address: <string> | default = ""] | ||
|
||
# port to advertise in consul (defaults to server.grpc-listen-port). |
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 this defaults to server.grpc-listen-port
, shouldn't we list the default for that port here rather than 0?
return nil, nil | ||
} | ||
|
||
reg := prometheus.WrapRegistererWithPrefix(t.Cfg.MetricsNamespace+"_", prometheus.DefaultRegisterer) |
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.
Will we get duplicate metrics registration panics here, as the prefix is the same as initRing
?
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.
🤔 not sure we have been using the registerer with prefix call multiple times in modules.go
so I thought it is a no-op here. OTH you are right if we ever call this method twice which is is not the case for distinct targets like ingest-limits
, however in all
it is something to investigate
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.
Follow up this not even a problem with target all
because we don't start multiple rings in the same binary there.
What this PR does / why we need it:
Adds a new ring for ingest-limits service instances to be used by the frontend's service discovery.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
deprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR