Skip to content
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

Move span.GetSamplerParams out of model/ into sampling/aggregator #6583

Merged
merged 5 commits into from
Jan 21, 2025
Merged

Move span.GetSamplerParams out of model/ into sampling/aggregator #6583

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
ad59def
fix: delegate some of span model parts into aggregator
Nabil-Salah Jan 21, 2025
1a7f5a8
enhance: turning getSamplerParams function to method for aggregator
Nabil-Salah Jan 21, 2025
7ea63fa
fix: use zap logger
Nabil-Salah Jan 21, 2025
c8b5cbb
feat: add TestGetSamplerParams test
Nabil-Salah Jan 21, 2025
318132b
Merge branch 'jaegertracing:main' into span_cleaner_model
Nabil-Salah Jan 21, 2025
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
36 changes: 35 additions & 1 deletion internal/sampling/samplingstrategy/adaptive/aggregator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package adaptive

import (
"strconv"
"sync"
"time"

Expand Down Expand Up @@ -150,9 +151,42 @@ func (a *aggregator) HandleRootSpan(span *span_model.Span) {
if service == "" || span.OperationName == "" {
return
}
samplerType, samplerParam := span.GetSamplerParams(a.postAggregator.logger)
samplerType, samplerParam := getSamplerParams(span, a.postAggregator.logger)
if samplerType == span_model.SamplerTypeUnrecognized {
return
}
a.RecordThroughput(service, span.OperationName, samplerType, samplerParam)
}

// GetSamplerParams returns the sampler.type and sampler.param value if they are valid.
func getSamplerParams(s *span_model.Span, logger *zap.Logger) (span_model.SamplerType, float64) {
samplerType := s.GetSamplerType()
if samplerType == span_model.SamplerTypeUnrecognized {
return span_model.SamplerTypeUnrecognized, 0
}
tag, ok := span_model.KeyValues(s.Tags).FindByKey(span_model.SamplerParamKey)
if !ok {
return span_model.SamplerTypeUnrecognized, 0
}
samplerParam, err := samplerParamToFloat(tag)
if err != nil {
logger.
With(zap.String("traceID", s.TraceID.String())).
With(zap.String("spanID", s.SpanID.String())).
Warn("sampler.param tag is not a number", zap.Any("tag", tag))
return span_model.SamplerTypeUnrecognized, 0
}
return samplerType, samplerParam
}

func samplerParamToFloat(samplerParamTag span_model.KeyValue) (float64, error) {
// The param could be represented as a string, an int, or a float
switch samplerParamTag.VType {
case span_model.Float64Type:
return samplerParamTag.Float64(), nil
case span_model.Int64Type:
return float64(samplerParamTag.Int64()), nil
default:
return strconv.ParseFloat(samplerParamTag.AsString(), 64)
}
}
122 changes: 122 additions & 0 deletions internal/sampling/samplingstrategy/adaptive/aggregator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package adaptive

import (
"net/http"
"strconv"
"testing"
"time"

Expand Down Expand Up @@ -186,3 +187,124 @@ func TestRecordThroughputFunc(t *testing.T) {
a.HandleRootSpan(span)
assert.EqualValues(t, 1, a.(*aggregator).currentThroughput["A"][http.MethodGet].Count)
}

func TestGetSamplerParams(t *testing.T) {
logger := zap.NewNop()

tests := []struct {
tags model.KeyValues
expectedType model.SamplerType
expectedParam float64
}{
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.String("sampler.param", "1e-05"),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 0.00001,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.Float64("sampler.param", 0.10404450002098709),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 0.10404450002098709,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.String("sampler.param", "0.10404450002098709"),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 0.10404450002098709,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.Int64("sampler.param", 1),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 1.0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "ratelimiting"),
model.String("sampler.param", "1"),
},
expectedType: model.SamplerTypeRateLimiting,
expectedParam: 1,
},
{
tags: model.KeyValues{
model.Float64("sampler.type", 1.5),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.String("sampler.param", "1"),
},
expectedType: model.SamplerTypeLowerBound,
expectedParam: 1,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.Int64("sampler.param", 1),
},
expectedType: model.SamplerTypeLowerBound,
expectedParam: 1,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.Float64("sampler.param", 0.5),
},
expectedType: model.SamplerTypeLowerBound,
expectedParam: 0.5,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.String("sampler.param", "not_a_number"),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "not_a_type"),
model.String("sampler.param", "not_a_number"),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
}

for i, test := range tests {
tt := test
t.Run(strconv.Itoa(i), func(t *testing.T) {
span := &model.Span{}
span.Tags = tt.tags
actualType, actualParam := getSamplerParams(span, logger)
assert.Equal(t, tt.expectedType, actualType)
assert.InDelta(t, tt.expectedParam, actualParam, 0.01)
})
}
}
36 changes: 0 additions & 36 deletions model/span.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ package model
import (
"encoding/gob"
"io"
"strconv"

"go.uber.org/zap"
)

type SamplerType int
Expand Down Expand Up @@ -142,36 +139,3 @@ func (s *Span) ReplaceParentID(newParentID SpanID) {
}
s.References = MaybeAddParentSpanID(s.TraceID, newParentID, s.References)
}

// GetSamplerParams returns the sampler.type and sampler.param value if they are valid.
func (s *Span) GetSamplerParams(logger *zap.Logger) (SamplerType, float64) {
samplerType := s.GetSamplerType()
if samplerType == SamplerTypeUnrecognized {
return SamplerTypeUnrecognized, 0
}
tag, ok := KeyValues(s.Tags).FindByKey(SamplerParamKey)
if !ok {
return SamplerTypeUnrecognized, 0
}
samplerParam, err := samplerParamToFloat(tag)
if err != nil {
logger.
With(zap.String("traceID", s.TraceID.String())).
With(zap.String("spanID", s.SpanID.String())).
Warn("sampler.param tag is not a number", zap.Any("tag", tag))
return SamplerTypeUnrecognized, 0
}
return samplerType, samplerParam
}

func samplerParamToFloat(samplerParamTag KeyValue) (float64, error) {
// The param could be represented as a string, an int, or a float
switch samplerParamTag.VType {
case Float64Type:
return samplerParamTag.Float64(), nil
case Int64Type:
return float64(samplerParamTag.Int64()), nil
default:
return strconv.ParseFloat(samplerParamTag.AsString(), 64)
}
}
125 changes: 1 addition & 124 deletions model/span_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,13 @@ package model_test
import (
"bytes"
"fmt"
"strconv"
"testing"
"time"

"github.com/gogo/protobuf/jsonpb"
"github.com/gogo/protobuf/proto"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/model"
)
Expand Down Expand Up @@ -165,7 +162,7 @@ func TestSpanIDUnmarshalJSONErrors(t *testing.T) {
func TestIsRPCClientServer(t *testing.T) {
span1 := &model.Span{
Tags: model.KeyValues{
model.String(model.SpanKindKey, trace.SpanKindClient.String()),
model.String(model.SpanKindKey, "client"),
},
}
assert.True(t, span1.IsRPCClient())
Expand Down Expand Up @@ -335,123 +332,3 @@ func BenchmarkBatchSerialization(b *testing.B) {
}
})
}

func TestGetSamplerParams(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

can you move the test to aggregator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by making it private it can't be tested alone but it's test will be part of HandleRootSpan test as it already uses it

Copy link
Member

Choose a reason for hiding this comment

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

if the test is in the same package it has access to private functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okaay you can merge code for this #6584 issue

and I will rebase and push changes

logger := zap.NewNop()
tests := []struct {
tags model.KeyValues
expectedType model.SamplerType
expectedParam float64
}{
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.String("sampler.param", "1e-05"),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 0.00001,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.Float64("sampler.param", 0.10404450002098709),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 0.10404450002098709,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.String("sampler.param", "0.10404450002098709"),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 0.10404450002098709,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.Int64("sampler.param", 1),
},
expectedType: model.SamplerTypeProbabilistic,
expectedParam: 1.0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "ratelimiting"),
model.String("sampler.param", "1"),
},
expectedType: model.SamplerTypeRateLimiting,
expectedParam: 1,
},
{
tags: model.KeyValues{
model.Float64("sampler.type", 1.5),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "probabilistic"),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.String("sampler.param", "1"),
},
expectedType: model.SamplerTypeLowerBound,
expectedParam: 1,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.Int64("sampler.param", 1),
},
expectedType: model.SamplerTypeLowerBound,
expectedParam: 1,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.Float64("sampler.param", 0.5),
},
expectedType: model.SamplerTypeLowerBound,
expectedParam: 0.5,
},
{
tags: model.KeyValues{
model.String("sampler.type", "lowerbound"),
model.String("sampler.param", "not_a_number"),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
{
tags: model.KeyValues{
model.String("sampler.type", "not_a_type"),
model.String("sampler.param", "not_a_number"),
},
expectedType: model.SamplerTypeUnrecognized,
expectedParam: 0,
},
}

for i, test := range tests {
tt := test
t.Run(strconv.Itoa(i), func(t *testing.T) {
span := &model.Span{}
span.Tags = tt.tags
actualType, actualParam := span.GetSamplerParams(logger)
assert.Equal(t, tt.expectedType, actualType)
assert.InDelta(t, tt.expectedParam, actualParam, 0.01)
})
}
}
Loading