Skip to content

Commit

Permalink
[agent] Remove baggage restriction manager (#6457)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?
- Cleaning up agent code.

## Description of the changes
- Remove baggage restriction manager code
- The thrift-gen/baggage still remains because there is a circular
dependency from OTEL collector
```
$ rm -rf thrift-gen/baggage

$ go run ./cmd/jaeger
../../../go/pkg/mod/github.com/open-telemetry/opentelemetry-collector-contrib/receiver/[email protected]/trace_receiver.go:27:2: no required module provides package github.com/jaegertracing/jaeger/thrift-gen/baggage; to add it:
	go get github.com/jaegertracing/jaeger/thrift-gen/baggage
```
This is because jaegerreceiver currently depends on jaeger/v1.62.0. Once
we release changes from this PR and OTEL is upgraded to the
corresponding Jaeger version, we should be able to delete
`thrift-gen/baggage`.


## How was this change tested?
- CI

---------

Signed-off-by: Mend Renovate <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Mend Renovate <[email protected]>
  • Loading branch information
yurishkuro and renovate-bot authored Jan 1, 2025
1 parent 2fe9e21 commit d0ad732
Show file tree
Hide file tree
Showing 14 changed files with 11 additions and 185 deletions.
4 changes: 2 additions & 2 deletions cmd/agent/app/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ type ServerConfiguration struct {
HostPort string `yaml:"hostPort" validate:"nonzero"`
}

// HTTPServerConfiguration holds config for a server providing sampling strategies and baggage restrictions to clients
// HTTPServerConfiguration holds config for a server providing sampling strategies to clients
type HTTPServerConfiguration struct {
HostPort string `yaml:"hostPort" validate:"nonzero"`
}
Expand Down Expand Up @@ -153,7 +153,7 @@ func (b *Builder) getProcessors(rep reporter.Reporter, mFactory metrics.Factory,
return retMe, nil
}

// GetHTTPServer creates an HTTP server that provides sampling strategies and baggage restrictions to client libraries.
// GetHTTPServer creates an HTTP server that provides sampling strategies to client libraries.
func (c HTTPServerConfiguration) getHTTPServer(manager configmanager.ClientConfigManager, mFactory metrics.Factory, logger *zap.Logger) *http.Server {
hostPort := c.HostPort
if hostPort == "" {
Expand Down
5 changes: 0 additions & 5 deletions cmd/agent/app/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/jaegertracing/jaeger/internal/metricstest"
"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/thrift-gen/baggage"
"github.com/jaegertracing/jaeger/thrift-gen/jaeger"
"github.com/jaegertracing/jaeger/thrift-gen/zipkincore"
)
Expand Down Expand Up @@ -194,10 +193,6 @@ func (fakeCollectorProxy) GetSamplingStrategy(_ context.Context, _ string) (*api
return nil, errors.New("no peers available")
}

func (fakeCollectorProxy) GetBaggageRestrictions(_ context.Context, _ string) ([]*baggage.BaggageRestriction, error) {
return nil, nil
}

func TestCreateCollectorProxy(t *testing.T) {
tests := []struct {
flags []string
Expand Down
7 changes: 0 additions & 7 deletions cmd/agent/app/configmanager/grpc/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@ package grpc

import (
"context"
"errors"
"fmt"

"google.golang.org/grpc"

"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/thrift-gen/baggage"
)

// ConfigManagerProxy returns sampling decisions from collector over gRPC.
Expand All @@ -34,8 +32,3 @@ func (s *ConfigManagerProxy) GetSamplingStrategy(ctx context.Context, serviceNam
}
return resp, nil
}

// GetBaggageRestrictions returns baggage restrictions from collector.
func (*ConfigManagerProxy) GetBaggageRestrictions(_ context.Context, _ string) ([]*baggage.BaggageRestriction, error) {
return nil, errors.New("baggage not implemented")
}
7 changes: 0 additions & 7 deletions cmd/agent/app/configmanager/grpc/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,6 @@ func TestSamplingManager_GetSamplingStrategy_error(t *testing.T) {
assert.ErrorContains(t, err, "failed to get sampling strategy")
}

func TestSamplingManager_GetBaggageRestrictions(t *testing.T) {
manager := NewConfigManager(nil)
rest, err := manager.GetBaggageRestrictions(context.Background(), "foo")
require.Nil(t, rest)
require.EqualError(t, err, "baggage not implemented")
}

type mockSamplingHandler struct{}

func (*mockSamplingHandler) GetSamplingStrategy(context.Context, *api_v2.SamplingStrategyParameters) (*api_v2.SamplingStrategyResponse, error) {
Expand Down
6 changes: 1 addition & 5 deletions cmd/agent/app/configmanager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,12 @@ import (
"context"

"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/thrift-gen/baggage"
)

// TODO this interface could be moved to pkg/clientcfg, along with grpc proxy,
// but not the metrics wrapper (because its metric names are specific to agent).

// ClientConfigManager decides:
// 1) which sampling strategy a given service should be using
// 2) which baggage restrictions a given service should be using.
// ClientConfigManager decides which sampling strategy a given service should be using.
type ClientConfigManager interface {
GetSamplingStrategy(ctx context.Context, serviceName string) (*api_v2.SamplingStrategyResponse, error)
GetBaggageRestrictions(ctx context.Context, serviceName string) ([]*baggage.BaggageRestriction, error)
}
18 changes: 0 additions & 18 deletions cmd/agent/app/configmanager/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/thrift-gen/baggage"
)

// configManagerMetrics holds metrics related to ClientConfigManager
Expand All @@ -18,12 +17,6 @@ type configManagerMetrics struct {

// Number of failed sampling rate responses from collector
SamplingFailures metrics.Counter `metric:"collector-proxy" tags:"result=err,endpoint=sampling"`

// Number of successful baggage restriction responses from collector
BaggageSuccess metrics.Counter `metric:"collector-proxy" tags:"result=ok,endpoint=baggage"`

// Number of failed baggage restriction responses from collector
BaggageFailures metrics.Counter `metric:"collector-proxy" tags:"result=err,endpoint=baggage"`
}

// ManagerWithMetrics is manager with metrics integration.
Expand All @@ -49,14 +42,3 @@ func (m *ManagerWithMetrics) GetSamplingStrategy(ctx context.Context, serviceNam
}
return r, err
}

// GetBaggageRestrictions returns baggage restrictions from server.
func (m *ManagerWithMetrics) GetBaggageRestrictions(ctx context.Context, serviceName string) ([]*baggage.BaggageRestriction, error) {
r, err := m.wrapped.GetBaggageRestrictions(ctx, serviceName)
if err != nil {
m.metrics.BaggageFailures.Inc(1)
} else {
m.metrics.BaggageSuccess.Inc(1)
}
return r, err
}
18 changes: 0 additions & 18 deletions cmd/agent/app/configmanager/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"github.com/jaegertracing/jaeger/internal/metricstest"
"github.com/jaegertracing/jaeger/pkg/testutils"
"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/thrift-gen/baggage"
)

type noopManager struct{}
Expand All @@ -26,13 +25,6 @@ func (noopManager) GetSamplingStrategy(_ context.Context, s string) (*api_v2.Sam
return &api_v2.SamplingStrategyResponse{StrategyType: api_v2.SamplingStrategyType_PROBABILISTIC}, nil
}

func (noopManager) GetBaggageRestrictions(_ context.Context, s string) ([]*baggage.BaggageRestriction, error) {
if s == "failed" {
return nil, errors.New("failed")
}
return []*baggage.BaggageRestriction{{BaggageKey: "foo"}}, nil
}

func TestMetrics(t *testing.T) {
tests := []struct {
expected []metricstest.ExpectedMetric
Expand All @@ -41,14 +33,10 @@ func TestMetrics(t *testing.T) {
{expected: []metricstest.ExpectedMetric{
{Name: "collector-proxy", Tags: map[string]string{"result": "ok", "endpoint": "sampling"}, Value: 1},
{Name: "collector-proxy", Tags: map[string]string{"result": "err", "endpoint": "sampling"}, Value: 0},
{Name: "collector-proxy", Tags: map[string]string{"result": "ok", "endpoint": "baggage"}, Value: 1},
{Name: "collector-proxy", Tags: map[string]string{"result": "err", "endpoint": "baggage"}, Value: 0},
}},
{expected: []metricstest.ExpectedMetric{
{Name: "collector-proxy", Tags: map[string]string{"result": "ok", "endpoint": "sampling"}, Value: 0},
{Name: "collector-proxy", Tags: map[string]string{"result": "err", "endpoint": "sampling"}, Value: 1},
{Name: "collector-proxy", Tags: map[string]string{"result": "ok", "endpoint": "baggage"}, Value: 0},
{Name: "collector-proxy", Tags: map[string]string{"result": "err", "endpoint": "baggage"}, Value: 1},
}, err: errors.New("failed")},
}

Expand All @@ -62,16 +50,10 @@ func TestMetrics(t *testing.T) {
s, err := mgr.GetSamplingStrategy(context.Background(), test.err.Error())
require.Nil(t, s)
require.EqualError(t, err, test.err.Error())
b, err := mgr.GetBaggageRestrictions(context.Background(), test.err.Error())
require.Nil(t, b)
require.EqualError(t, err, test.err.Error())
} else {
s, err := mgr.GetSamplingStrategy(context.Background(), "")
require.NoError(t, err)
require.NotNil(t, s)
b, err := mgr.GetBaggageRestrictions(context.Background(), "")
require.NoError(t, err)
require.NotNil(t, b)
}
metricsFactory.AssertCounterMetrics(t, test.expected...)
})
Expand Down
2 changes: 1 addition & 1 deletion cmd/agent/app/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func AddFlags(flags *flag.FlagSet) {
flags.String(
httpServerHostPort,
defaultHTTPServerHostPort,
"host:port of the http server (e.g. for /sampling point and /baggageRestrictions endpoint)")
"host:port of the http server (e.g. for /sampling endpoint)")

for _, p := range defaultProcessors {
prefix := fmt.Sprintf(processorPrefixFmt, p.model, p.protocol)
Expand Down
2 changes: 1 addition & 1 deletion cmd/agent/app/httpserver/srv.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
)

// NewHTTPServer creates a new server that hosts an HTTP/JSON endpoint for clients
// to query for sampling strategies and baggage restrictions.
// to query for sampling strategies.
func NewHTTPServer(hostPort string, manager configmanager.ClientConfigManager, mFactory metrics.Factory, logger *zap.Logger) *http.Server {
handler := clientcfghttp.NewHTTPHandler(clientcfghttp.HTTPHandlerParams{
ConfigManager: manager,
Expand Down
1 change: 0 additions & 1 deletion cmd/collector/app/server/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ func serveHTTP(server *http.Server, listener net.Listener, params *HTTPServerPar
cfgHandler := clientcfgHandler.NewHTTPHandler(clientcfgHandler.HTTPHandlerParams{
ConfigManager: &clientcfgHandler.ConfigManager{
SamplingProvider: params.SamplingProvider,
// TODO provide baggage manager
},
MetricsFactory: params.MetricsFactory,
BasePath: "/api",
Expand Down
11 changes: 0 additions & 11 deletions pkg/clientcfg/clientcfghttp/cfgmgr.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,17 @@ package clientcfghttp

import (
"context"
"errors"

"github.com/jaegertracing/jaeger/cmd/collector/app/sampling/samplingstrategy"
"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/thrift-gen/baggage"
)

// ConfigManager implements ClientConfigManager.
type ConfigManager struct {
SamplingProvider samplingstrategy.Provider
BaggageManager baggage.BaggageRestrictionManager
}

// GetSamplingStrategy implements ClientConfigManager.GetSamplingStrategy.
func (c *ConfigManager) GetSamplingStrategy(ctx context.Context, serviceName string) (*api_v2.SamplingStrategyResponse, error) {
return c.SamplingProvider.GetSamplingStrategy(ctx, serviceName)
}

// GetBaggageRestrictions implements ClientConfigManager.GetBaggageRestrictions.
func (c *ConfigManager) GetBaggageRestrictions(ctx context.Context, serviceName string) ([]*baggage.BaggageRestriction, error) {
if c.BaggageManager == nil {
return nil, errors.New("baggage restrictions not implemented")
}
return c.BaggageManager.GetBaggageRestrictions(ctx, serviceName)
}
26 changes: 0 additions & 26 deletions pkg/clientcfg/clientcfghttp/cfgmgr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/stretchr/testify/require"

"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/thrift-gen/baggage"
)

type mockSamplingProvider struct {
Expand All @@ -30,40 +29,15 @@ func (*mockSamplingProvider) Close() error {
return nil
}

type mockBaggageMgr struct {
baggageResponse []*baggage.BaggageRestriction
}

func (m *mockBaggageMgr) GetBaggageRestrictions(context.Context, string /* serviceName */) ([]*baggage.BaggageRestriction, error) {
if m.baggageResponse == nil {
return nil, errors.New("no mock response provided")
}
return m.baggageResponse, nil
}

func TestConfigManager(t *testing.T) {
bgm := &mockBaggageMgr{}
mgr := &ConfigManager{
SamplingProvider: &mockSamplingProvider{
samplingResponse: &api_v2.SamplingStrategyResponse{},
},
BaggageManager: bgm,
}
t.Run("GetSamplingStrategy", func(t *testing.T) {
r, err := mgr.GetSamplingStrategy(context.Background(), "foo")
require.NoError(t, err)
assert.Equal(t, api_v2.SamplingStrategyResponse{}, *r)
})
t.Run("GetBaggageRestrictions", func(t *testing.T) {
expResp := []*baggage.BaggageRestriction{}
bgm.baggageResponse = expResp
r, err := mgr.GetBaggageRestrictions(context.Background(), "foo")
require.NoError(t, err)
assert.Equal(t, expResp, r)
})
t.Run("GetBaggageRestrictionsError", func(t *testing.T) {
mgr.BaggageManager = nil
_, err := mgr.GetBaggageRestrictions(context.Background(), "foo")
require.EqualError(t, err, "baggage restrictions not implemented")
})
}
28 changes: 1 addition & 27 deletions pkg/clientcfg/clientcfghttp/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type HTTPHandlerParams struct {
}

// HTTPHandler implements endpoints for used by Jaeger clients to retrieve client configuration,
// such as sampling and baggage restrictions.
// such as sampling strategies.
type HTTPHandler struct {
params HTTPHandlerParams
metrics struct {
Expand All @@ -48,9 +48,6 @@ type HTTPHandler struct {
// Number of good sampling requests against the old endpoint / using Thrift 0.9.2 enum codes
LegacySamplingRequestSuccess metrics.Counter `metric:"http-server.requests" tags:"type=sampling-legacy"`

// Number of good baggage requests
BaggageRequestSuccess metrics.Counter `metric:"http-server.requests" tags:"type=baggage"`

// Number of bad requests (400s)
BadRequest metrics.Counter `metric:"http-server.errors" tags:"status=4xx,source=all"`

Expand Down Expand Up @@ -92,10 +89,6 @@ func (h *HTTPHandler) RegisterRoutes(router *mux.Router) {
h.serveSamplingHTTP(w, r, h.encodeProto)
},
).Methods(http.MethodGet)

router.HandleFunc(prefix+"/baggageRestrictions", func(w http.ResponseWriter, r *http.Request) {
h.serveBaggageHTTP(w, r)
}).Methods(http.MethodGet)
}

// RegisterRoutes registers configuration handlers with HTTP Router.
Expand Down Expand Up @@ -179,25 +172,6 @@ func (h *HTTPHandler) encodeProto(strategy *api_v2.SamplingStrategyResponse) ([]
return []byte(str), nil
}

func (h *HTTPHandler) serveBaggageHTTP(w http.ResponseWriter, r *http.Request) {
service, err := h.serviceFromRequest(w, r)
if err != nil {
return
}
resp, err := h.params.ConfigManager.GetBaggageRestrictions(r.Context(), service)
if err != nil {
h.metrics.CollectorProxyFailures.Inc(1)
http.Error(w, fmt.Sprintf("collector error: %+v", err), http.StatusInternalServerError)
return
}
// NB. it's literally impossible for this Marshal to fail
jsonBytes, _ := json.Marshal(resp)
if err = h.writeJSON(w, jsonBytes); err != nil {
return
}
h.metrics.BaggageRequestSuccess.Inc(1)
}

var samplingStrategyTypes = []api_v2.SamplingStrategyType{
api_v2.SamplingStrategyType_PROBABILISTIC,
api_v2.SamplingStrategyType_RATE_LIMITING,
Expand Down
Loading

0 comments on commit d0ad732

Please sign in to comment.