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

feat: Add has-root-span operator to rules #814

Merged
merged 5 commits into from
Aug 1, 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
4 changes: 2 additions & 2 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,13 @@ jobs:
test:
docker:
- image: cimg/go:1.19
- image: redis:6
- image: redis:6.2
steps:
- checkout
- restore_cache:
keys:
- v1-dockerize-{{ checksum "Makefile" }}
- v1-dockerize-
- run: make verify-licenses
- run: make dockerize
- save_cache:
key: v1-dockerize-{{ checksum "Makefile" }}
Expand All @@ -80,6 +79,7 @@ jobs:
- restore_cache:
keys:
- v3-go-mod-{{ checksum "go.sum" }}
- run: make verify-licenses
- run: make test
- save_cache:
key: v3-go-mod-{{ checksum "go.sum" }}
Expand Down
4 changes: 2 additions & 2 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func TestReadRulesConfig(t *testing.T) {
assert.NoError(t, err)
switch r := d.(type) {
case *RulesBasedSamplerConfig:
assert.Len(t, r.Rules, 5)
assert.Len(t, r.Rules, 6)

var rule *RulesBasedSamplerRule

Expand All @@ -356,7 +356,7 @@ func TestReadRulesConfig(t *testing.T) {
assert.Equal(t, 5, rule.SampleRate)
assert.Equal(t, "span", rule.Scope)

rule = r.Rules[4]
rule = r.Rules[5]
assert.Equal(t, 10, rule.SampleRate)
assert.Equal(t, "", rule.Scope)

Expand Down
13 changes: 7 additions & 6 deletions config/metadata/rulesMeta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ groups:
`Samplers` maps targets to sampler configurations. Each target is a
Honeycomb environment (or a dataset for Honeycomb Classic keys).
The value is the sampler to use for that target. A `__default__`
target is required. The target called `__default__` will be used
target is required. The target called `__default__` will be used
for any target that is not explicitly listed.

The targets are determined by examining the API key used to send the
Expand All @@ -33,7 +33,7 @@ groups:
summary: is the sample rate to use.
description: >
The sample rate to use. It indicates a ratio, where one sample trace
is kept for every N traces seen. For example, a `SampleRate` of `30`
is kept for every N traces seen. For example, a `SampleRate` of `30`
will keep 1 out of every 30 traces. The choice on whether to keep any
specific trace is random, so the rate is approximate.

Expand Down Expand Up @@ -78,19 +78,19 @@ groups:
will be handed to the Dynamic Sampler. The combination of values from
all of these fields should reflect how interesting the trace is
compared to another.

When choosing field names for `FieldList`, a good field selection
has consistent values for high-frequency, boring traffic, and
unique values for outliers and interesting traffic. Including
an error field, or something like `HTTP status code`, is an
excellent choice. Using fields with very high cardinality,
like `k8s.pod.id`, is a bad choice.

If the combination of fields essentially makes each trace unique,
then the Dynamic Sampler will sample everything. If the combination
of fields is not unique enough, then you will not be guaranteed
samples of the most interesting traces.

As an example, consider as a good set of fields: the combination of
`HTTP endpoint` (high-frequency and boring), `HTTP method`, and
`status code` (normally boring but can become interesting when
Expand All @@ -102,7 +102,7 @@ groups:
combination of `HTTP endpoint`, `status code`, and `pod id`, since it
would result in keys that are all unique, and therefore result in
sampling 100% of traces.

For example, rather than a set of fields, using only the `HTTP
endpoint` field is a **bad** choice, as it is not unique enough, and
therefore interesting traces, like traces that experienced a `500`,
Expand Down Expand Up @@ -593,6 +593,7 @@ groups:
- does-not-contain
- exists
- not-exists
- has-root-span
summary: is the comparison operator to use.
description: >
The comparison operator to use. String comparisons are case-sensitive.
Expand Down
12 changes: 8 additions & 4 deletions config/sampler_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const (
StartsWith = "starts-with"
Exists = "exists"
NotExists = "not-exists"
HasRootSpan = "has-root-span"
)

// The json tags in this file are used for conversion from the old format (see tools/convert for details).
Expand Down Expand Up @@ -249,6 +250,9 @@ func (r *RulesBasedSamplerCondition) setMatchesFunction() error {
if err != nil {
return err
}
case HasRootSpan:
// this is evaluated at the trace level, so we don't need to do anything here
return nil
default:
return fmt.Errorf("unknown operator '%s'", r.Operator)
}
Expand Down Expand Up @@ -302,7 +306,7 @@ func tryConvertToString(v any) (string, bool) {
return fmt.Sprintf("%v", v), true
}

func tryConvertToBool(v any) bool {
func TryConvertToBool(v any) bool {
value, ok := tryConvertToString(v)
if !ok {
return false
Expand Down Expand Up @@ -490,20 +494,20 @@ func setCompareOperators(r *RulesBasedSamplerCondition, condition string) error
return nil
}
case "bool":
conditionValue := tryConvertToBool(r.Value)
conditionValue := TryConvertToBool(r.Value)

switch condition {
case NEQ:
r.Matches = func(spanValue any, exists bool) bool {
if n := tryConvertToBool(spanValue); exists {
if n := TryConvertToBool(spanValue); exists {
return n != conditionValue
}
return false
}
return nil
case EQ:
r.Matches = func(spanValue any, exists bool) bool {
if n := tryConvertToBool(spanValue); exists {
if n := TryConvertToBool(spanValue); exists {
return n == conditionValue
}
return false
Expand Down
8 changes: 8 additions & 0 deletions rules_complete.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,14 @@ Samplers:
- Field: trace.parent_id
Operator: =
Value: root
- Name: drop incomplete traces from the buggy service
Drop: true
Conditions:
- Operator: has-root-span
Value: false
- Field: service.name
Operator: =
Value: buggy-service
# This is the default rule (with no conditions) and therefore
# sets the sample rate used if no other rule matches.
- Name: default rule
Expand Down
14 changes: 14 additions & 0 deletions sample/rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,20 @@ func ruleMatchesTrace(t *types.Trace, rule *config.RulesBasedSamplerRule, checkN
var matched int

for _, condition := range rule.Conditions {
// This condition is evaluated for the trace as a whole.
// If RootSpan is nil, it means the trace timer has fired or the trace has been
// ejected from the cache before the root span has arrived.
if condition.Operator == config.HasRootSpan {
if (t.RootSpan != nil) == config.TryConvertToBool(condition.Value) {
kentquirk marked this conversation as resolved.
Show resolved Hide resolved
matched++
continue
} else {
// if HasRootSpan is one of the conditions and it didn't match,
// there's no need to check the rest of the conditions.
return false
}
}

span:
for _, span := range t.GetSpans() {
value, exists := extractValueFromSpan(span, condition, checkNestedFields)
Expand Down
96 changes: 96 additions & 0 deletions sample/rules_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,99 @@ func TestRules(t *testing.T) {
ExpectedKeep: false,
ExpectedRate: 0,
},
{
Rules: &config.RulesBasedSamplerConfig{
Rules: []*config.RulesBasedSamplerRule{
{
Name: "Check that root span is missing",
Drop: true,
SampleRate: 0,
Conditions: []*config.RulesBasedSamplerCondition{
{
Operator: config.HasRootSpan,
Value: false,
},
},
},
},
},
Spans: []*types.Span{
{
Event: types.Event{
Data: map[string]interface{}{
"trace.trace_id": "12345",
"trace.span_id": "654321",
"trace.parent_id": "54321",
"test": int64(2),
},
},
},
{
Event: types.Event{
Data: map[string]interface{}{
"trace.trace_id": "12345",
"trace.span_id": "754321",
"trace.parent_id": "54321",
"test": int64(3),
},
},
},
},
ExpectedName: "Check that root span is missing",
ExpectedKeep: false,
ExpectedRate: 0,
},
{
Rules: &config.RulesBasedSamplerConfig{
Rules: []*config.RulesBasedSamplerRule{
{
Name: "Check that root span is present",
SampleRate: 99,
Conditions: []*config.RulesBasedSamplerCondition{
{
Operator: config.HasRootSpan,
Value: true,
},
},
},
},
},
Spans: []*types.Span{
{
Event: types.Event{
Data: map[string]interface{}{
"trace.trace_id": "12345",
"trace.span_id": "54321",
"meta.span_count": int64(2),
"test": int64(2),
},
},
},
{
Event: types.Event{
Data: map[string]interface{}{
"trace.trace_id": "12345",
"trace.span_id": "654321",
"trace.parent_id": "54321",
"test": int64(2),
},
},
},
{
Event: types.Event{
Data: map[string]interface{}{
"trace.trace_id": "12345",
"trace.span_id": "754321",
"trace.parent_id": "54321",
"test": int64(3),
},
},
},
},
ExpectedName: "Check that root span is present",
ExpectedKeep: true,
ExpectedRate: 99,
},
kentquirk marked this conversation as resolved.
Show resolved Hide resolved
}

for _, d := range data {
Expand All @@ -604,6 +697,9 @@ func TestRules(t *testing.T) {

for _, span := range d.Spans {
trace.AddSpan(span)
if _, ok := span.Data["trace.parent_id"]; ok == false {
trace.RootSpan = span
}
}

rate, keep, reason, key := sampler.GetSampleRate(trace)
Expand Down