From 424274aab59a05c2865420c005b1173e1779e990 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 13 Jan 2025 17:15:02 -0500 Subject: [PATCH] Bugfix: Default step for gRPC streaming query range queries (#4546) * foo Signed-off-by: Joe Elliott * fix test Signed-off-by: Joe Elliott * fix an issue where exemplars were not being dumped correctly Signed-off-by: Joe Elliott * changelog Signed-off-by: Joe Elliott * lint Signed-off-by: Joe Elliott --------- Signed-off-by: Joe Elliott --- CHANGELOG.md | 3 +++ cmd/tempo-cli/cmd-query-metrics-query-range.go | 1 - cmd/tempo-cli/shared.go | 9 ++++++--- pkg/api/http.go | 8 +++++++- pkg/api/http_test.go | 4 +++- 5 files changed, 19 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c58fc223b36..5c8b9f65171 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## main / unreleased +* [BUGFIX] Choose a default step for a gRPC streaming query range request if none is provided. [#4546](https://github.com/grafana/tempo/pull/4546) (@joe-elliott) + Fix an issue where the tempo-cli was not correctly dumping exemplar results. + # v2.7.0 * [CHANGE] Disable gRPC compression in the querier and distributor for performance reasons [#4429](https://github.com/grafana/tempo/pull/4429) (@carles-grafana) diff --git a/cmd/tempo-cli/cmd-query-metrics-query-range.go b/cmd/tempo-cli/cmd-query-metrics-query-range.go index 7f15b766b81..15bc997f584 100644 --- a/cmd/tempo-cli/cmd-query-metrics-query-range.go +++ b/cmd/tempo-cli/cmd-query-metrics-query-range.go @@ -61,7 +61,6 @@ func (cmd *metricsQueryCmd) Run(_ *globalOptions) error { Query: cmd.TraceQL, Start: uint64(start), End: uint64(end), - Step: uint64(5 * time.Second), } if cmd.UseGRPC { diff --git a/cmd/tempo-cli/shared.go b/cmd/tempo-cli/shared.go index 1bd13a3df60..dba63a971bd 100644 --- a/cmd/tempo-cli/shared.go +++ b/cmd/tempo-cli/shared.go @@ -2,13 +2,14 @@ package main import ( "context" - "encoding/json" "errors" "fmt" "sort" "strconv" "time" + "github.com/gogo/protobuf/jsonpb" + "github.com/gogo/protobuf/proto" "github.com/google/uuid" "github.com/grafana/tempo/pkg/boundedwaitgroup" "github.com/grafana/tempo/tempodb/backend" @@ -124,8 +125,10 @@ func loadBlock(r backend.Reader, c backend.Compactor, tenantID string, id backen }, nil } -func printAsJSON[T any](value T) error { - traceJSON, err := json.Marshal(value) +func printAsJSON(value proto.Message) error { + m := jsonpb.Marshaler{} + + traceJSON, err := m.MarshalToString(value) if err != nil { return err } diff --git a/pkg/api/http.go b/pkg/api/http.go index 2a4ed06c0f2..6dc3b2596a0 100644 --- a/pkg/api/http.go +++ b/pkg/api/http.go @@ -456,10 +456,16 @@ func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequ return req } + // 0 is an invalid step, so we need to calculate it if it's not provided + step := searchReq.Step + if step == 0 { + step = traceql.DefaultQueryRangeStep(searchReq.Start, searchReq.End) + } + qb := newQueryBuilder("") qb.addParam(urlParamStart, strconv.FormatUint(searchReq.Start, 10)) qb.addParam(urlParamEnd, strconv.FormatUint(searchReq.End, 10)) - qb.addParam(urlParamStep, time.Duration(searchReq.Step).String()) + qb.addParam(urlParamStep, time.Duration(step).String()) qb.addParam(QueryModeKey, searchReq.QueryMode) // New RF1 params qb.addParam(urlParamBlockID, searchReq.BlockID) diff --git a/pkg/api/http_test.go b/pkg/api/http_test.go index 6f340ebd38e..01516319364 100644 --- a/pkg/api/http_test.go +++ b/pkg/api/http_test.go @@ -704,7 +704,9 @@ func TestQueryRangeRoundtrip(t *testing.T) { }{ { name: "empty", - req: &tempopb.QueryRangeRequest{}, + req: &tempopb.QueryRangeRequest{ + Step: uint64(time.Second), // you can't actually roundtrip an empty query b/c Build/Parse will force a default step + }, }, { name: "not empty!",