-
Notifications
You must be signed in to change notification settings - Fork 482
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
Add range in traceql search for vulture #2475
Conversation
tested in ops overnight and things look good |
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.
approving to unblock but please change the metric reason.
cmd/tempo-vulture/main.go
Outdated
@@ -262,6 +263,7 @@ func pushMetrics(metrics traceMetrics) { | |||
metricTracesErrors.WithLabelValues("incorrectresult").Add(float64(metrics.incorrectResult)) | |||
metricTracesErrors.WithLabelValues("missingspans").Add(float64(metrics.missingSpans)) | |||
metricTracesErrors.WithLabelValues("notfound_search").Add(float64(metrics.notFoundSearch)) | |||
metricTracesErrors.WithLabelValues("notfound_search").Add(float64(metrics.notFoundTraceQL)) |
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.
we should change the reason label from notfound_search
to notfound_traceql
or something
pkg/util/client.go
Outdated
|
||
func (c *Client) SearchTraceQLWithRange(query string, start int64, end int64) (*tempopb.SearchResponse, error) { | ||
m := &tempopb.SearchResponse{} | ||
_, err := c.getFor(c.BaseURL+"/api/search?q="+url.QueryEscape(query)+"&start="+strconv.FormatInt(start, 10)+"&end="+strconv.FormatInt(end, 10), m) |
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 there are more correct ways to build a query string like this. likely using the url object and then asking it to encode all at once, but this is fine given what we're doing here
return m, nil | ||
} | ||
|
||
func (c *Client) buildQueryURL(queryType string, query string, start int64, end int64) string { |
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.
@joe-elliott not sure if this is better... I can just revert it to the old ways of building url if not
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.
up to you. if it works it works. i do think this is "more correct"
What this PR does: Vulture was doing test traceql queries for but since time range was not added in the query, it was getting false errors
Which issue(s) this PR fixes:
Fixes https://github.com/grafana/tempo-squad/issues/240
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]