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

[processor/probabilisticsampler] fix panic when sampling on non-Bytes log record attribute #18223

Closed
Closed
Show file tree
Hide file tree
Changes from 4 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
16 changes: 16 additions & 0 deletions .chloggen/fix_probabilisticsamplerprocessor-panic.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: probabilisticsamplerprocessor

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Fix panic in probabilisticsampler processor if a log attribute to be sampled on is of type String.

# One or more tracking issues related to the change
issues: [18222]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
12 changes: 11 additions & 1 deletion processor/probabilisticsamplerprocessor/logsprocessor.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,17 @@ func (lsp *logSamplerProcessor) processLogs(ctx context.Context, ld plog.Logs) (
if lidBytes == nil && lsp.samplingSource != "" {
if value, ok := l.Attributes().Get(lsp.samplingSource); ok {
tagPolicyValue = lsp.samplingSource
lidBytes = value.Bytes().AsRaw()
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved

switch value.Type() {
case pcommon.ValueTypeStr:
Copy link
Author

Choose a reason for hiding this comment

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

I could do with some input here.. I'm not sure if we should be handling string types without understanding the encoding. See PR discussion for more context.

Copy link
Member

Choose a reason for hiding this comment

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

I would say that this doesn't matter much, as a hash will be calculated based on the value. If it's string or int, it doesn't matter, as long as they are all converted to bytes in the end. Additionally, it doesn't matter if we are using the right encoding for the string, as long as all values are byte encoded in the same way. If we were to use this information for other than just calculating a hash, it would certainly be important to properly handle the encoding.

lidBytes = []byte(value.Str())
case pcommon.ValueTypeBytes:
lidBytes = value.Bytes().AsRaw()
default:
lsp.logger.Warn("incompatible log record attribute, only String or Bytes supported; skipping log record",
Copy link
Author

Choose a reason for hiding this comment

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

In this case the user has configured a log record attribute that is not a supported type. Unfortunately, the RemoveIf functions used by this processor don't return errors, so I figure the only sane thing here without a bigger refactor is to log something. Is WARN the appropriate severity for a misconfigured processor?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the only value that should not be supported is boolean. Otherwise, the probability provided by the user will be skewed by the fact that booleans can only have two values: there won't be a good distribution of values to make a proper probability.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there is a method named, AsString which works regardless the type since it just calls fmt.Sprint ?

Would that work here instead?

zap.String("log_record_attribute", lsp.samplingSource), zap.Stringer("attribute_type", value.Type()))
}

}
}
priority := lsp.scaledSamplingRate
Expand Down
35 changes: 34 additions & 1 deletion processor/probabilisticsamplerprocessor/logsprocessor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,15 @@ func TestLogsSampling(t *testing.T) {
},
received: 25,
},
{
name: "sampling a string attribute",
cfg: &Config{
SamplingPercentage: 50,
AttributeSource: recordAttributeSource,
FromAttribute: "bar",
},
received: 22,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -149,10 +158,13 @@ func TestLogsSampling(t *testing.T) {
ib := byte(i)
traceID := [16]byte{0, 0, 0, 0, 0, 0, 0, 0, ib, ib, ib, ib, ib, ib, ib, ib}
record.SetTraceID(traceID)
// set half of records with a foo attribute
// set half of records with a foo bytes attribute
if i%2 == 0 {
b := record.Attributes().PutEmptyBytes("foo")
b.FromRaw(traceID[:])
} else {
// set the other half of records with a bar string attribute
record.Attributes().PutStr("bar", string(traceID[:]))
}
// set a fourth of records with a priority attribute
if i%4 == 0 {
Expand All @@ -170,3 +182,24 @@ func TestLogsSampling(t *testing.T) {
})
}
}

func TestLogsSamplingInvalidType(t *testing.T) {
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved
sink := new(consumertest.LogsSink)
cfg := &Config{
SamplingPercentage: 100,
AttributeSource: recordAttributeSource,
FromAttribute: "bool_attr",
}
processor, err := newLogsProcessor(context.Background(), processortest.NewNopCreateSettings(), sink, cfg)
require.NoError(t, err)
logs := plog.NewLogs()
lr := logs.ResourceLogs().AppendEmpty().ScopeLogs().AppendEmpty().LogRecords()
record := lr.AppendEmpty()
record.Attributes().PutBool("bool_attr", true)

// When processing a log event with an unsupported type, the event is not processed.
err = processor.ConsumeLogs(context.Background(), logs)
require.NoError(t, err)
sunk := sink.AllLogs()
assert.Equal(t, 0, len(sunk))
}
Loading