Skip to content

Commit

Permalink
Remove Logger parameter in adaptive/aggregator.go (#6586)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?
- #6584

## Description of the changes
- edit `HandleRootSpan` and it's mocks to use `aggregator` parameter
logger

## How was this change tested?
- `make lint test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

Signed-off-by: nabil salah <[email protected]>
  • Loading branch information
Nabil-Salah authored Jan 21, 2025
1 parent 156a59d commit d6ed101
Show file tree
Hide file tree
Showing 7 changed files with 15 additions and 17 deletions.
2 changes: 1 addition & 1 deletion cmd/collector/app/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (c *Collector) Start(options *flags.CollectorOptions) error {
var additionalProcessors []ProcessSpan
if c.samplingAggregator != nil {
additionalProcessors = append(additionalProcessors, func(span *model.Span, _ /* tenant */ string) {
c.samplingAggregator.HandleRootSpan(span, c.logger)
c.samplingAggregator.HandleRootSpan(span)
})
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/collector/app/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func (t *mockAggregator) RecordThroughput(string /* service */, string /* operat
t.callCount.Add(1)
}

func (t *mockAggregator) HandleRootSpan(*model.Span, *zap.Logger) {
func (t *mockAggregator) HandleRootSpan(*model.Span) {
t.callCount.Add(1)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func (tp *traceProcessor) processTraces(_ context.Context, td ptrace.Traces) (pt
if span.Process == nil {
span.Process = batch.Process
}
tp.aggregator.HandleRootSpan(span, tp.telset.Logger)
tp.aggregator.HandleRootSpan(span)
}
}
return td, nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ type notClosingAgg struct{}

func (*notClosingAgg) Close() error { return errors.New("not closing") }

func (*notClosingAgg) HandleRootSpan(*model.Span, *zap.Logger) {}
func (*notClosingAgg) HandleRootSpan(*model.Span) {}
func (*notClosingAgg) RecordThroughput(string, string, model.SamplerType, float64) {}
func (*notClosingAgg) Start() {}

Expand Down
4 changes: 2 additions & 2 deletions internal/sampling/samplingstrategy/adaptive/aggregator.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (a *aggregator) Close() error {
return nil
}

func (a *aggregator) HandleRootSpan(span *span_model.Span, logger *zap.Logger) {
func (a *aggregator) HandleRootSpan(span *span_model.Span) {
// simply checking parentId to determine if a span is a root span is not sufficient. However,
// we can be sure that only a root span will have sampler tags.
if span.ParentSpanID() != span_model.NewSpanID(0) {
Expand All @@ -150,7 +150,7 @@ func (a *aggregator) HandleRootSpan(span *span_model.Span, logger *zap.Logger) {
if service == "" || span.OperationName == "" {
return
}
samplerType, samplerParam := span.GetSamplerParams(logger)
samplerType, samplerParam := span.GetSamplerParams(a.postAggregator.logger)
if samplerType == span_model.SamplerTypeUnrecognized {
return
}
Expand Down
16 changes: 8 additions & 8 deletions internal/sampling/samplingstrategy/adaptive/aggregator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,28 +121,28 @@ func TestRecordThroughput(t *testing.T) {

// Testing non-root span
span := &model.Span{References: []model.SpanRef{{SpanID: model.NewSpanID(1), RefType: model.ChildOf}}}
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
require.Empty(t, a.(*aggregator).currentThroughput)

// Testing span with service name but no operation
span.References = []model.SpanRef{}
span.Process = &model.Process{
ServiceName: "A",
}
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
require.Empty(t, a.(*aggregator).currentThroughput)

// Testing span with service name and operation but no probabilistic sampling tags
span.OperationName = http.MethodGet
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
require.Empty(t, a.(*aggregator).currentThroughput)

// Testing span with service name, operation, and probabilistic sampling tags
span.Tags = model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.String("sampler.param", "0.001"),
}
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
assert.EqualValues(t, 1, a.(*aggregator).currentThroughput["A"][http.MethodGet].Count)
}

Expand All @@ -162,27 +162,27 @@ func TestRecordThroughputFunc(t *testing.T) {

// Testing non-root span
span := &model.Span{References: []model.SpanRef{{SpanID: model.NewSpanID(1), RefType: model.ChildOf}}}
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
require.Empty(t, a.(*aggregator).currentThroughput)

// Testing span with service name but no operation
span.References = []model.SpanRef{}
span.Process = &model.Process{
ServiceName: "A",
}
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
require.Empty(t, a.(*aggregator).currentThroughput)

// Testing span with service name and operation but no probabilistic sampling tags
span.OperationName = http.MethodGet
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
require.Empty(t, a.(*aggregator).currentThroughput)

// Testing span with service name, operation, and probabilistic sampling tags
span.Tags = model.KeyValues{
model.String("sampler.type", "probabilistic"),
model.String("sampler.param", "0.001"),
}
a.HandleRootSpan(span, logger)
a.HandleRootSpan(span)
assert.EqualValues(t, 1, a.(*aggregator).currentThroughput["A"][http.MethodGet].Count)
}
4 changes: 1 addition & 3 deletions internal/sampling/samplingstrategy/aggregator.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ package samplingstrategy
import (
"io"

"go.uber.org/zap"

"github.com/jaegertracing/jaeger/model"
)

Expand All @@ -18,7 +16,7 @@ type Aggregator interface {

// The HandleRootSpan function processes a span, checking if it's a root span.
// If it is, it extracts sampler parameters, then calls RecordThroughput.
HandleRootSpan(span *model.Span, logger *zap.Logger)
HandleRootSpan(span *model.Span)

// RecordThroughput records throughput for an operation for aggregation.
RecordThroughput(service, operation string, samplerType model.SamplerType, probability float64)
Expand Down

0 comments on commit d6ed101

Please sign in to comment.