Skip to content

Commit

Permalink
[samplers/jaegerremote] Change parser to support enums as both string…
Browse files Browse the repository at this point in the history
…s and numbers (#3183)

* squash

* delint

* undo .gitignore

* Move changelog entry to unreleased

---------

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Tyler Yahn <codingalias@gmail.com>
  • Loading branch information
3 people authored Feb 8, 2023
1 parent 0a2a048 commit 6178903
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 3 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,17 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

- Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068)

### Changed

- `samplers/jaegerremote`: change to use protobuf parser instead of encoding/json to accept enums as strings. (#3183)

## [1.14.0/0.39.0/0.8.0] - 2023-02-07

### Changed

- Change `runtime.uptime` instrument in `go.opentelemetry.io/contrib/instrumentation/runtime` from `Int64ObservableUpDownCounter` to `Int64ObservableCounter`,
since the value is monotonic. (#3347)
- `samplers/jaegerremote`: change to use protobuf parser instead of encoding/json to accept enums as strings. (#3183)

### Fixed

Expand Down
2 changes: 1 addition & 1 deletion samplers/jaegerremote/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Jaeger Remote Sampler

This package implements [Jaeger remote sampler](https://www.jaegertracing.io/docs/latest/sampling/#collector-sampling-configuration).
This package implements [Jaeger remote sampler](https://www.jaegertracing.io/docs/latest/sampling/#remote-sampling).
Remote sampler allows defining sampling configuration for services at the backend, at the granularity of service + endpoint.
When using the Jaeger backend, the sampling configuration can come from two sources:

Expand Down
10 changes: 8 additions & 2 deletions samplers/jaegerremote/sampler_remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package jaegerremote // import "go.opentelemetry.io/contrib/samplers/jaegerremote"

import (
"encoding/json"
"bytes"
"fmt"
"io"
"net/http"
Expand All @@ -26,6 +26,8 @@ import (
"sync/atomic"
"time"

"github.com/gogo/protobuf/jsonpb"

jaeger_api_v2 "go.opentelemetry.io/contrib/samplers/jaegerremote/internal/proto-gen/jaeger-idl/proto/api_v2"
"go.opentelemetry.io/otel/sdk/trace"
)
Expand Down Expand Up @@ -311,7 +313,11 @@ type samplingStrategyParserImpl struct{}

func (p *samplingStrategyParserImpl) Parse(response []byte) (interface{}, error) {
strategy := new(jaeger_api_v2.SamplingStrategyResponse)
if err := json.Unmarshal(response, strategy); err != nil {
// Official Jaeger Remote Sampling protocol contains enums encoded as strings.
// Legacy protocol contains enums as numbers.
// Gogo's jsonpb module can parse either format.
// Cf. https://github.com/open-telemetry/opentelemetry-go-contrib/issues/3184
if err := jsonpb.Unmarshal(bytes.NewReader(response), strategy); err != nil {
return nil, err
}
return strategy, nil
Expand Down
52 changes: 52 additions & 0 deletions samplers/jaegerremote/sampler_remote_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -535,3 +535,55 @@ func getSamplingStrategyResponse(strategyType jaeger_api_v2.SamplingStrategyType
}
return nil
}

func TestSamplingStrategyParserImpl(t *testing.T) {
assertProbabilistic := func(t *testing.T, s *jaeger_api_v2.SamplingStrategyResponse) {
require.NotNil(t, s.GetProbabilisticSampling(), "output: %+v", s)
require.EqualValues(t, 0.42, s.GetProbabilisticSampling().GetSamplingRate(), "output: %+v", s)
}
assertRateLimiting := func(t *testing.T, s *jaeger_api_v2.SamplingStrategyResponse) {
require.NotNil(t, s.GetRateLimitingSampling(), "output: %+v", s)
require.EqualValues(t, 42, s.GetRateLimitingSampling().GetMaxTracesPerSecond(), "output: %+v", s)
}
tests := []struct {
name string
json string
assert func(t *testing.T, s *jaeger_api_v2.SamplingStrategyResponse)
}{
{
name: "official JSON probabilistic",
json: `{"strategyType":"PROBABILISTIC","probabilisticSampling":{"samplingRate":0.42}}`,
assert: assertProbabilistic,
},
{
name: "official JSON rate limiting",
json: `{"strategyType":"RATE_LIMITING","rateLimitingSampling":{"maxTracesPerSecond":42}}`,
assert: assertRateLimiting,
},
{
name: "legacy JSON probabilistic",
json: `{"strategyType":0,"probabilisticSampling":{"samplingRate":0.42}}`,
assert: assertProbabilistic,
},
{
name: "legacy JSON rate limiting",
json: `{"strategyType":1,"rateLimitingSampling":{"maxTracesPerSecond":42}}`,
assert: assertRateLimiting,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
val, err := new(samplingStrategyParserImpl).Parse([]byte(test.json))
require.NoError(t, err)
s := val.(*jaeger_api_v2.SamplingStrategyResponse)
test.assert(t, s)
})
}
}

func TestSamplingStrategyParserImpl_Error(t *testing.T) {
json := `{"strategyType":"foo_bar","probabilisticSampling":{"samplingRate":0.42}}`
val, err := new(samplingStrategyParserImpl).Parse([]byte(json))
require.Error(t, err, "output: %+v", val)
require.Contains(t, err.Error(), `unknown value "foo_bar"`)
}

0 comments on commit 6178903

Please sign in to comment.