-
Notifications
You must be signed in to change notification settings - Fork 450
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
Das client using chunks metric #2599
Conversation
Fixes NIT-2742 |
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! Does this PR replace #2549?
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
Oops, I had meant for this PR to be based on that PR, changed the base now. Thanks. |
The base branch was changed.
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.
Didn't review yet. Marking as approved because it has previous approvals that were dismissed when it's parent changed. That way it should come up in my searches for approved PRs
In promql you can make a query to discover and show the rates for the metrics with different names like this:
|
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.
One comment but generally LGTM
url: target, | ||
signer: signer, | ||
chunkSize: uint64(chunkSize), | ||
metricName: metricsutil.CanonicalizeMetricName(url.Hostname()), |
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.
It looks like CanonicalizeMetricName doesn't filter out periods, which don't seem to be an allowed Prometheus metric name but are of course common in URLs.
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 it does replace them. The regex is picking characters that don't match the range, which doesn't include .
, and replacing them with _
.
// Prometheus metric names must contain only chars [a-zA-Z0-9:_]
func CanonicalizeMetricName(metric string) string {
invalidPromCharRegex := regexp.MustCompile(`[^a-zA-Z0-9:_]+`)
return invalidPromCharRegex.ReplaceAllString(metric, "_")
}
If you look at the Nova dashboard, first panel, you can see the dots in domains have been replaced with _
.
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.
Prometheus metrics are really for things that should be tracked long term, but legacy/chunked is just a transition between new and old, so better to just emit an info log when using legacy (no log when doing chunked)
Will redo as a new PR to just log when we are using the legacy method. |
This PR adds a metric on the batch poster side to indicate which DAS backends are using the chunked store method and which are using the legacy method.
The metrics that are added are: