Skip to content

[metricstore] Remove min step API from metricstore #8425

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.

Sign up for GitHub

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

Merged
mahadzaryab1 merged 5 commits into from
Apr 23, 2026
Merged

[metricstore] Remove min step API from metricstore #8425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0e6d65
Remove GetCallRates From Interface
mahadzaryab1 Apr 23, 2026
f486ad9
Regenerate Mocks
mahadzaryab1 Apr 23, 2026
f0ef61c
Update Callsites
mahadzaryab1 Apr 23, 2026
c7faed8
fix
mahadzaryab1 Apr 23, 2026
7c7e0d5
Fix
mahadzaryab1 Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions cmd/jaeger/internal/extension/jaegerquery/internal/http_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ func (aH *APIHandler) RegisterRoutes(router *http.ServeMux) {
aH.handleFunc(router, aH.latencies, http.MethodGet, "/metrics/latencies")
aH.handleFunc(router, aH.calls, http.MethodGet, "/metrics/calls")
aH.handleFunc(router, aH.errors, http.MethodGet, "/metrics/errors")
aH.handleFunc(router, aH.minStep, http.MethodGet, "/metrics/minstep")
aH.handleFunc(router, aH.getQualityMetrics, http.MethodGet, "/quality-metrics")
Comment on lines 119 to 122

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes the /api/metrics/minstep endpoint, but docker-compose/monitor/README.md still documents and demonstrates it. Please update/remove that documentation (or add a note about removal) to avoid broken examples for users.

Copilot uses AI. Check for mistakes.
}

Expand Down Expand Up @@ -379,18 +378,6 @@ func (aH *APIHandler) errors(w http.ResponseWriter, r *http.Request) {
})
}

func (aH *APIHandler) minStep(w http.ResponseWriter, r *http.Request) {
minStep, err := aH.metricsQueryService.GetMinStepDuration(r.Context(), &metricstore.MinStepDurationQueryParameters{})
if aH.handleError(w, err, http.StatusInternalServerError) {
return
}

structuredRes := structuredResponse{
Data: minStep.Milliseconds(),
}
aH.writeJSON(w, r, &structuredRes)
}

func (aH *APIHandler) metrics(w http.ResponseWriter, r *http.Request, getMetrics func(context.Context, metricstore.BaseQueryParameters) (*metrics.MetricFamily, error)) {
requestParams, err := aH.queryParser.parseMetricsQueryParams(r)
if aH.handleError(w, err, http.StatusBadRequest) {
Comment on lines 381 to 383

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minStep handler was removed, but RegisterRoutes still registers /metrics/minstep via aH.minStep, which will not compile. Remove the route registration (and any related endpoint expectations) to fully drop the API, or reintroduce the handler if the endpoint should remain supported.

Copilot uses AI. Check for mistakes.
Expand Down
32 changes: 0 additions & 32 deletions cmd/jaeger/internal/extension/jaegerquery/internal/http_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,13 +974,6 @@ func TestMetricsReaderError(t *testing.T) {
mockedResponse: nil,
wantErrorMessage: "error fetching call rates",
},
{
urlPath: "/api/metrics/minstep",
mockedQueryMethod: "GetMinStepDuration",
mockedQueryMethodParamType: "*metricstore.MinStepDurationQueryParameters",
mockedResponse: time.Duration(0),
wantErrorMessage: "error fetching min step duration",
},
} {
t.Run(tc.wantErrorMessage, func(t *testing.T) {
// Prepare
Expand Down Expand Up @@ -1017,11 +1010,6 @@ func TestMetricsQueryDisabled(t *testing.T) {
urlPath: "/api/metrics/latencies?service=emailservice&quantile=0.95",
wantErrorMessage: "metrics querying is currently disabled",
},
{
name: "metrics query disabled error returned when fetching min step duration",
urlPath: "/api/metrics/minstep",
wantErrorMessage: "metrics querying is currently disabled",
},
} {
t.Run(tc.name, func(t *testing.T) {
// Test
Expand All @@ -1034,26 +1022,6 @@ func TestMetricsQueryDisabled(t *testing.T) {
}
}

func TestGetMinStep(t *testing.T) {
metricsReader := &metricsmocks.Reader{}
ts := initializeTestServer(t, HandlerOptions.MetricsQueryService(metricsReader))
defer ts.server.Close()
// Prepare
metricsReader.On(
"GetMinStepDuration",
mock.Anything,
mock.AnythingOfType("*metricstore.MinStepDurationQueryParameters"),
).Return(5*time.Millisecond, nil).Once()

// Test
var response structuredResponse
err := getJSON(ts.server.URL+"/api/metrics/minstep", &response)

// Verify
require.NoError(t, err)
assert.InDelta(t, float64(5), response.Data, 0.01)
}

// getJSON fetches a JSON document from a server via HTTP GET
func getJSON(url string, out any) error {
return getJSONCustomHeaders(url, make(map[string]string), out)
Comment on lines 1025 to 1027

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestMetricsQueryDisabled still calls /api/metrics/minstep and expects the "metrics querying is currently disabled" error. Since the minstep endpoint is being removed, this test case should be deleted or updated to assert the new expected behavior (e.g., 404 / route not found) after the route is removed.

Copilot uses AI. Check for mistakes.
Expand Down
6 changes: 0 additions & 6 deletions internal/storage/metricstore/disabled/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package disabled

import (
"context"
"time"

"github.com/jaegertracing/jaeger/internal/proto-gen/api_v2/metrics"
"github.com/jaegertracing/jaeger/internal/storage/v1/api/metricstore"
Expand Down Expand Up @@ -46,8 +45,3 @@ func (*MetricsReader) GetCallRates(context.Context, *metricstore.CallRateQueryPa
func (*MetricsReader) GetErrorRates(context.Context, *metricstore.ErrorRateQueryParameters) (*metrics.MetricFamily, error) {
return nil, ErrDisabled
}

// GetMinStepDuration gets the minimum step duration (the smallest possible duration between two data points in a time series) supported.
func (*MetricsReader) GetMinStepDuration(context.Context, *metricstore.MinStepDurationQueryParameters) (time.Duration, error) {
return 0, ErrDisabled
}
12 changes: 0 additions & 12 deletions internal/storage/metricstore/disabled/reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,3 @@ func TestGetErrorRates(t *testing.T) {
require.ErrorIs(t, err, ErrDisabled)
require.EqualError(t, err, ErrDisabled.Error())
}

func TestGetMinStepDurations(t *testing.T) {
reader, err := NewMetricsReader()
require.NoError(t, err)
require.NotNil(t, reader)

qParams := &metricstore.MinStepDurationQueryParameters{}
r, err := reader.GetMinStepDuration(context.Background(), qParams)
assert.Zero(t, r)
require.ErrorIs(t, err, ErrDisabled)
require.EqualError(t, err, ErrDisabled.Error())
}
7 changes: 0 additions & 7 deletions internal/storage/metricstore/elasticsearch/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import (
"github.com/jaegertracing/jaeger/internal/storage/v1/api/metricstore"
)

const minStep = time.Millisecond

// MetricsReader orchestrates metrics queries by:
// 1. Calculating time ranges from query parameters.
// 2. Delegating query construction and execution to Query.
Expand Down Expand Up @@ -166,11 +164,6 @@ func (r MetricsReader) GetErrorRates(ctx context.Context, params *metricstore.Er
return CalculateErrorRates(rawErrorsMetrics, callRateMetrics, params.BaseQueryParameters, timeRange), nil
}

// GetMinStepDuration returns the minimum step duration.
func (MetricsReader) GetMinStepDuration(_ context.Context, _ *metricstore.MinStepDurationQueryParameters) (time.Duration, error) {
return minStep, nil
}

// bucketsToPoints is a helper function for getting points value from ES AGG bucket
func bucketsToPoints(buckets []*elastic.AggregationBucketHistogramItem, valueExtractor func(*elastic.AggregationBucketHistogramItem) float64) []*Pair {
var points []*Pair
Expand Down
9 changes: 0 additions & 9 deletions internal/storage/metricstore/elasticsearch/reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,15 +878,6 @@ func TestGetErrorRates(t *testing.T) {
}
}

func TestGetMinStepDuration(t *testing.T) {
mockServer := startMockEsServer(t, "", mockEsValidResponse)
defer mockServer.Close()
reader, _ := setupMetricsReaderFromServer(t, mockServer)
minStep, err := reader.GetMinStepDuration(context.Background(), &metricstore.MinStepDurationQueryParameters{})
require.NoError(t, err)
assert.Equal(t, time.Millisecond, minStep)
}

func TestGetCallRateBucketsToPoints_ErrorCases(t *testing.T) {
tests := []struct {
name string
Expand Down
9 changes: 0 additions & 9 deletions internal/storage/metricstore/prometheus/metricstore/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ import (
"github.com/jaegertracing/jaeger/internal/telemetry/otelsemconv"
)

const (
minStep = time.Millisecond
)

type (
// MetricsReader is a Prometheus metrics reader.
MetricsReader struct {
Expand Down Expand Up @@ -254,11 +250,6 @@ func (m MetricsReader) GetErrorRates(ctx context.Context, requestParams *metrics
return errorMetrics, nil
}

// GetMinStepDuration gets the minimum step duration (the smallest possible duration between two data points in a time series) supported.
func (MetricsReader) GetMinStepDuration(_ context.Context, _ *metricstore.MinStepDurationQueryParameters) (time.Duration, error) {
return minStep, nil
}

// executeQuery executes a query against a Prometheus-compliant metrics backend.
func (m MetricsReader) executeQuery(ctx context.Context, p metricsQueryParams) (*metrics.MetricFamily, error) {
if p.GroupByOperation {
Expand Down
21 changes: 0 additions & 21 deletions internal/storage/metricstore/prometheus/metricstore/reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package metricstore
import (
"context"
"io"
"net"
"net/http"
"net/http/httptest"
"net/url"
Expand Down Expand Up @@ -91,26 +90,6 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) {
assert.Nil(t, reader)
}

func TestGetMinStepDuration(t *testing.T) {
params := metricstore.MinStepDurationQueryParameters{}
logger := zap.NewNop()
tracer, _, closer := tracerProvider(t)
defer closer()
listener, err := net.Listen("tcp", "localhost:")
require.NoError(t, err)
assert.NotNil(t, listener)

reader, err := NewMetricsReader(config.Configuration{
ServerURL: "http://" + listener.Addr().String(),
ConnectTimeout: defaultTimeout,
}, logger, tracer, nil)
require.NoError(t, err)

minStep, err := reader.GetMinStepDuration(context.Background(), &params)
require.NoError(t, err)
assert.Equal(t, time.Millisecond, minStep)
}

func TestMetricsServerError(t *testing.T) {
endTime := time.Now()
lookback := time.Minute
Expand Down
6 changes: 0 additions & 6 deletions internal/storage/v1/api/metricstore/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ type Reader interface {
// GetErrorRates gets the error rate metrics for a given list of services grouped by service
// and optionally grouped by operation.
GetErrorRates(ctx context.Context, params *ErrorRateQueryParameters) (*metrics.MetricFamily, error)
// GetMinStepDuration gets the min time resolution supported by the backing metrics store,
// e.g. 10s means the backend can only return data points that are at least 10s apart, not closer.
GetMinStepDuration(ctx context.Context, params *MinStepDurationQueryParameters) (time.Duration, error)
}

// BaseQueryParameters contains the common set of parameters used by all metrics queries:
Expand Down Expand Up @@ -69,6 +66,3 @@ type CallRateQueryParameters struct {
type ErrorRateQueryParameters struct {
BaseQueryParameters
}

// MinStepDurationQueryParameters contains the parameters required for fetching the minimum step duration.
type MinStepDurationQueryParameters struct{}
26 changes: 8 additions & 18 deletions internal/storage/v1/api/metricstore/metricstoremetrics/decorator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@ import (

// ReadMetricsDecorator wraps a metricstore.Reader and collects metrics around each read operation.
type ReadMetricsDecorator struct {
reader metricstore.Reader
getLatenciesMetrics *queryMetrics
getCallRatesMetrics *queryMetrics
getErrorRatesMetrics *queryMetrics
getMinStepDurationMetrics *queryMetrics
reader metricstore.Reader
getLatenciesMetrics *queryMetrics
getCallRatesMetrics *queryMetrics
getErrorRatesMetrics *queryMetrics
}

type queryMetrics struct {
Expand All @@ -41,11 +40,10 @@ func (q *queryMetrics) emit(err error, latency time.Duration) {
// NewReadMetricsDecorator returns a new ReadMetricsDecorator.

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment says NewReadMetricsDecorator but the function is named NewReaderDecorator. Please update the comment to match the actual function name (or rename the function) so GoDoc and search results are accurate.

Suggested change
// NewReadMetricsDecorator returns a new ReadMetricsDecorator.
// NewReaderDecorator returns a new ReadMetricsDecorator.

Copilot uses AI. Check for mistakes.
func NewReaderDecorator(reader metricstore.Reader, metricsFactory metrics.Factory) *ReadMetricsDecorator {
return &ReadMetricsDecorator{
reader: reader,
getLatenciesMetrics: buildQueryMetrics("get_latencies", metricsFactory),
getCallRatesMetrics: buildQueryMetrics("get_call_rates", metricsFactory),
getErrorRatesMetrics: buildQueryMetrics("get_error_rates", metricsFactory),
getMinStepDurationMetrics: buildQueryMetrics("get_min_step_duration", metricsFactory),
reader: reader,
getLatenciesMetrics: buildQueryMetrics("get_latencies", metricsFactory),
getCallRatesMetrics: buildQueryMetrics("get_call_rates", metricsFactory),
getErrorRatesMetrics: buildQueryMetrics("get_error_rates", metricsFactory),
}
}

Expand Down Expand Up @@ -79,11 +77,3 @@ func (m *ReadMetricsDecorator) GetErrorRates(ctx context.Context, params *metric
m.getErrorRatesMetrics.emit(err, time.Since(start))
return retMe, err
}

// GetMinStepDuration implements metricstore.Reader#GetMinStepDuration
func (m *ReadMetricsDecorator) GetMinStepDuration(ctx context.Context, params *metricstore.MinStepDurationQueryParameters) (time.Duration, error) {
start := time.Now()
retMe, err := m.reader.GetMinStepDuration(ctx, params)
m.getMinStepDurationMetrics.emit(err, time.Since(start))
return retMe, err
}
39 changes: 12 additions & 27 deletions internal/storage/v1/api/metricstore/metricstoremetrics/decorator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"context"
"errors"
"testing"
"time"

"github.com/stretchr/testify/assert"

Expand Down Expand Up @@ -39,21 +38,14 @@ func TestSuccessfulUnderlyingCalls(t *testing.T) {
Return(&protometrics.MetricFamily{}, nil)
mrs.GetErrorRates(context.Background(), gerParams)

msdParams := &metricstore.MinStepDurationQueryParameters{}
mockReader.On("GetMinStepDuration", context.Background(), msdParams).
Return(time.Second, nil)
mrs.GetMinStepDuration(context.Background(), msdParams)

counters, gauges := mf.Snapshot()
wantCounts := map[string]int64{
"requests|operation=get_latencies|result=ok": 1,
"requests|operation=get_latencies|result=err": 0,
"requests|operation=get_call_rates|result=ok": 1,
"requests|operation=get_call_rates|result=err": 0,
"requests|operation=get_error_rates|result=ok": 1,
"requests|operation=get_error_rates|result=err": 0,
"requests|operation=get_min_step_duration|result=ok": 1,
"requests|operation=get_min_step_duration|result=err": 0,
"requests|operation=get_latencies|result=ok": 1,
"requests|operation=get_latencies|result=err": 0,
"requests|operation=get_call_rates|result=ok": 1,
"requests|operation=get_call_rates|result=err": 0,
"requests|operation=get_error_rates|result=ok": 1,
"requests|operation=get_error_rates|result=err": 0,
}

// This is not exhaustive.
Expand Down Expand Up @@ -112,21 +104,14 @@ func TestFailingUnderlyingCalls(t *testing.T) {
Return(&protometrics.MetricFamily{}, errors.New("failure"))
mrs.GetErrorRates(context.Background(), gerParams)

msdParams := &metricstore.MinStepDurationQueryParameters{}
mockReader.On("GetMinStepDuration", context.Background(), msdParams).
Return(time.Second, errors.New("failure"))
mrs.GetMinStepDuration(context.Background(), msdParams)

counters, gauges := mf.Snapshot()
wantCounts := map[string]int64{
"requests|operation=get_latencies|result=ok": 0,
"requests|operation=get_latencies|result=err": 1,
"requests|operation=get_call_rates|result=ok": 0,
"requests|operation=get_call_rates|result=err": 1,
"requests|operation=get_error_rates|result=ok": 0,
"requests|operation=get_error_rates|result=err": 1,
"requests|operation=get_min_step_duration|result=ok": 0,
"requests|operation=get_min_step_duration|result=err": 1,
"requests|operation=get_latencies|result=ok": 0,
"requests|operation=get_latencies|result=err": 1,
"requests|operation=get_call_rates|result=ok": 0,
"requests|operation=get_call_rates|result=err": 1,
"requests|operation=get_error_rates|result=ok": 0,
"requests|operation=get_error_rates|result=err": 1,
}

// This is not exhaustive.
Expand Down
Loading
Loading