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.

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

Fix tracing module's Jaeger sampling flag #3251

Merged
merged 2 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions js/modules/k6/experimental/tracing/propagator.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ const (
// "0 value is valid and means “root span” (when not ignored)"
JaegerRootSpanID = "0"

// JaegerSampledTraceFlag is the trace-flag value for an unsampled trace.
JaegerSampledTraceFlag = "0"
// JaegerUnsampledTraceFlag is the trace-flag value for an unsampled trace.
JaegerUnsampledTraceFlag = "0"

// JaegerUnsampledTraceFlag is the trace-flag value for a sampled trace.
JaegerUnsampledTraceFlag = "1"
// JaegerSampledTraceFlag is the trace-flag value for a sampled trace.
JaegerSampledTraceFlag = "1"
Comment on lines +72 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit strange to me. It is a boolean, isn't possible to have a single value so a single source of truth?

Copy link
Member Author

@oleiade oleiade Aug 7, 2023

Choose a reason for hiding this comment

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

This is a small simplification. The sampled/not sampled flag is specified as part of a set of flags: a bit within a byte.

As per the specification it could have a range of values. It just so happens that we're only using the sample flag at the moment, and encode it, at the end of the chain as a string, so keeping the constant as a string is a shortcut that avoids adding conversion code.

Happy to change it to numerical values and do the encoding instead, if you prefer; but I would do it in a different PR (this is a bug fix). Just let me know 👍

)

// JaegerPropagator is a Propagator for the Jaeger trace context header
Expand Down
112 changes: 112 additions & 0 deletions js/modules/k6/experimental/tracing/propagator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package tracing

import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestPropagate(t *testing.T) {
t.Parallel()

traceID := "abc123"

t.Run("W3C Propagator", func(t *testing.T) {
t.Parallel()

sampler := mockSampler{decision: true}
propagator := NewW3CPropagator(sampler)

gotHeader, gotErr := propagator.Propagate(traceID)

assert.NoError(t, gotErr)
assert.Contains(t, gotHeader, W3CHeaderName)

//nolint:staticcheck // as traceparent is not a canonical header
headerContent := gotHeader[W3CHeaderName][0]
assert.True(t, strings.HasPrefix(headerContent, W3CVersion+"-"+traceID+"-"))
})

t.Run("W3C propagator with sampled trace", func(t *testing.T) {
t.Parallel()

sampler := mockSampler{decision: true}
propagator := NewW3CPropagator(sampler)

gotHeader, gotErr := propagator.Propagate(traceID)
require.NoError(t, gotErr)
require.Contains(t, gotHeader, W3CHeaderName)

//nolint:staticcheck // as traceparent is not a canonical header
assert.True(t, strings.HasSuffix(gotHeader[W3CHeaderName][0], "-01"))
})

t.Run("W3C propagator with unsampled trace", func(t *testing.T) {
t.Parallel()

sampler := mockSampler{decision: false}
propagator := NewW3CPropagator(sampler)

gotHeader, gotErr := propagator.Propagate(traceID)
require.NoError(t, gotErr)
require.Contains(t, gotHeader, W3CHeaderName)

//nolint:staticcheck // as traceparent is not a canonical header
assert.True(t, strings.HasSuffix(gotHeader[W3CHeaderName][0], "-00"))
})

t.Run("Jaeger Propagator", func(t *testing.T) {
t.Parallel()

sampler := mockSampler{decision: true}
propagator := NewJaegerPropagator(sampler)

gotHeader, gotErr := propagator.Propagate(traceID)

assert.NoError(t, gotErr)
assert.Contains(t, gotHeader, JaegerHeaderName)

//nolint:staticcheck // as traceparent is not a canonical header
headerContent := gotHeader[JaegerHeaderName][0]
assert.True(t, strings.HasPrefix(headerContent, traceID+":"))
assert.True(t, strings.HasSuffix(headerContent, ":0:1"))
})

t.Run("Jaeger propagator with sampled trace", func(t *testing.T) {
t.Parallel()

sampler := mockSampler{decision: true}
propagator := NewJaegerPropagator(sampler)

gotHeader, gotErr := propagator.Propagate(traceID)
require.NoError(t, gotErr)
require.Contains(t, gotHeader, JaegerHeaderName)

//nolint:staticcheck // as traceparent is not a canonical header
assert.True(t, strings.HasSuffix(gotHeader[JaegerHeaderName][0], ":1"))
})

t.Run("Jaeger propagator with unsampled trace", func(t *testing.T) {
t.Parallel()

sampler := mockSampler{decision: false}
propagator := NewJaegerPropagator(sampler)

gotHeader, gotErr := propagator.Propagate(traceID)
require.NoError(t, gotErr)
require.Contains(t, gotHeader, JaegerHeaderName)

//nolint:staticcheck // as traceparent is not a canonical header
assert.True(t, strings.HasSuffix(gotHeader[JaegerHeaderName][0], ":0"))
})
}

type mockSampler struct {
decision bool
}

func (m mockSampler) ShouldSample() bool {
return m.decision
}