-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
app/vmalert: support DNS SRV record in -remoteWrite.url
#6299
Conversation
lib/httputils/statconn.go
Outdated
// and register stats metrics for conns. | ||
func GetStatDialFunc(metricPrefix string) func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
metricSetRegister.Do(func() { | ||
dialsTotal = metrics.NewCounter(fmt.Sprintf(`%s_dials_total`, metricPrefix)) |
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.
do we need custom metric name for each component?
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.
Why not? I think it is useful to have these metrics, unless they start to collide.
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.
imho finding needed data on grafana is easier, when you have a static metric name with different label values
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.
I think differently, when you know which metric you wanna use, having different metricName or different labels with same metricName are the same.
But when you can't remember the exact metricName, using auto-complete, having specific metricName prefix like remotewrite
or remoteread
can help to find metrics that component exposes for specific feature.
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.
Would be nice to add few lines to vmalert docs about this new feature, besides flag description update.
lib/httputils/statconn.go
Outdated
// and register stats metrics for conns. | ||
func GetStatDialFunc(metricPrefix string) func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
metricSetRegister.Do(func() { | ||
dialsTotal = metrics.NewCounter(fmt.Sprintf(`%s_dials_total`, metricPrefix)) |
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.
Why not? I think it is useful to have these metrics, unless they start to collide.
I think flag description is pretty clear for vmalert here, since DNS SRV record is a common term and extra chapter in doc won't contain any extra message. |
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! Please see my comments and a follow-up commit 4d9d6d7. Would appreciate your opinion on it
8e2b71d
to
691bf6f
Compare
Signed-off-by: hagen1778 <roman@victoriametrics.com>
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
part of #6053, supports [DNS SRV](https://en.wikipedia.org/wiki/SRV_record) address in `-remoteWrite.url` command-line option. (cherry picked from commit d7b5062)
Describe Your Changes
part of #6053,
supports DNS SRV address in
-remoteWrite.url
command-line option.Checklist
The following checks are mandatory: