Skip to content

Commit 500a6bf

Browse files
kinyoklionkinyoklion
authored
fix: Fix an issue where logging configuration could over-match for log records without a message or severity. (#261)
## Summary The CustomSampler errantly checked if message and severity were of the correct type before attempting to match them. If they were not, then they matched. The simplest solutions is to not do the check. Alternatively it could do the check, and when the type is not string they do not match. ## How did you test this change? Unit tests. ## Other After this PR is non-js SDKs should have their JSON files updated. I did not update in this PR because I didn't want to trigger a release of those SDKs. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Ensure log rules requiring message/severity only match when the record has a string message/severity; add scenarios and test helpers across SDKs. > > - **Sampling logic**: > - Go (`go/internal/otel/custom_sampler.go`): In `matchesLogConfig`, require `record.Body()` to be `KindString` for message matching; return false otherwise. > - JS/TS (Node, Shared, Highlight Run): Simplify `matchesLogConfig` by removing permissive typeof gates; Shared explicitly rejects non-string `record.body` for message matching. > - **Tests**: > - Add log scenarios that verify configs with both `message` and `severityText` do not match when logs are empty, only have message, or only have severity (in Go/Node/Shared/Highlight Run JSONs). > - Introduce helper to construct `ReadableLogRecord` from scenario input in Node/Shared tests and update usages. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 67c9367. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent b0f3bb1 commit 500a6bf

File tree

10 files changed

+422
-32
lines changed

10 files changed

+422
-32
lines changed

‎go/internal/otel/custom_sampler.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -625,10 +625,12 @@ func (cs *CustomSampler) matchesLogConfig(
625625

626626
// Check message if defined
627627
if messageConfig := config.GetMessage(); !isMatchConfigEmpty(&messageConfig) {
628-
if record.Body().Kind() == log.KindString {
629-
if !matchesValue(cs, &messageConfig, record.Body().AsString()) {
630-
return false
631-
}
628+
// We only support string message bodies for now.
629+
if record.Body().Kind() != log.KindString {
630+
return false
631+
}
632+
if !matchesValue(cs, &messageConfig, record.Body().AsString()) {
633+
return false
632634
}
633635
}
634636

‎go/internal/otel/log-test-scenarios.json‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -420,5 +420,102 @@
420420
}
421421
}
422422
]
423+
},
424+
{
425+
"description": "Should not match an empty log when message and severity are defined",
426+
"samplingConfig": {
427+
"logs": [
428+
{
429+
"message": {
430+
"regexValue": "Database connection .*"
431+
},
432+
"severityText": {
433+
"matchValue": "error"
434+
},
435+
"samplingRatio": 90
436+
}
437+
]
438+
},
439+
"inputLog": {},
440+
"samplerFunctionCases": [
441+
{
442+
"type": "always",
443+
"expected_result": {
444+
"sample": true
445+
}
446+
},
447+
{
448+
"type": "never",
449+
"expected_result": {
450+
"sample": true
451+
}
452+
}
453+
]
454+
},
455+
{
456+
"description": "Should not match a log with a message when message and severity are defined",
457+
"samplingConfig": {
458+
"logs": [
459+
{
460+
"message": {
461+
"regexValue": "Database connection .*"
462+
},
463+
"severityText": {
464+
"matchValue": "error"
465+
},
466+
"samplingRatio": 90
467+
}
468+
]
469+
},
470+
"inputLog": {
471+
"message": "Database connection failed: timeout"
472+
},
473+
"samplerFunctionCases": [
474+
{
475+
"type": "always",
476+
"expected_result": {
477+
"sample": true
478+
}
479+
},
480+
{
481+
"type": "never",
482+
"expected_result": {
483+
"sample": true
484+
}
485+
}
486+
]
487+
},
488+
{
489+
"description": "Should not match a log with only severity when message and severity are defined",
490+
"samplingConfig": {
491+
"logs": [
492+
{
493+
"message": {
494+
"regexValue": "Database connection .*"
495+
},
496+
"severityText": {
497+
"matchValue": "error"
498+
},
499+
"samplingRatio": 90
500+
}
501+
]
502+
},
503+
"inputLog": {
504+
"severityText": "error"
505+
},
506+
"samplerFunctionCases": [
507+
{
508+
"type": "always",
509+
"expected_result": {
510+
"sample": true
511+
}
512+
},
513+
{
514+
"type": "never",
515+
"expected_result": {
516+
"sample": true
517+
}
518+
}
519+
]
423520
}
424521
]

‎sdk/@launchdarkly/observability-node/src/otel/sampling/CustomSampler.test.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,14 @@ const runTestSpanScenarios = (scenarios: SpanTestScenario[]) => {
150150
})
151151
}
152152

153+
const toReadableLogRecord = (log: LogTestScenario['inputLog']) => {
154+
return {
155+
severityText: log.severityText,
156+
body: log.message,
157+
attributes: log.attributes,
158+
} as ReadableLogRecord
159+
}
160+
153161
const runTestLogScenarios = (scenarios: LogTestScenario[]) => {
154162
scenarios.forEach((scenario) => {
155163
scenario.samplerFunctionCases.forEach((samplerCase) => {
@@ -162,7 +170,7 @@ const runTestLogScenarios = (scenarios: LogTestScenario[]) => {
162170
expect(sampler.isSamplingEnabled()).toBe(true)
163171

164172
const result = sampler.sampleLog(
165-
scenario.inputLog as ReadableLogRecord,
173+
toReadableLogRecord(scenario.inputLog),
166174
)
167175

168176
expect(result.sample).toBe(samplerCase.expected_result.sample)

‎sdk/@launchdarkly/observability-node/src/otel/sampling/CustomSampler.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -232,20 +232,14 @@ export class CustomSampler implements ExportSampler {
232232
): boolean {
233233
if (config.severityText) {
234234
const severityText = record.severityText
235-
if (
236-
typeof severityText === 'string' &&
237-
!this.matchesValue(config.severityText, severityText)
238-
) {
235+
if (!this.matchesValue(config.severityText, severityText)) {
239236
return false
240237
}
241238
}
242239

243240
if (config.message) {
244241
const message = record.body
245-
if (
246-
typeof message === 'string' &&
247-
!this.matchesValue(config.message, message)
248-
) {
242+
if (!this.matchesValue(config.message, message)) {
249243
return false
250244
}
251245
}

‎sdk/@launchdarkly/observability-node/src/otel/sampling/log-test-scenarios.json‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,5 +388,102 @@
388388
}
389389
}
390390
]
391+
},
392+
{
393+
"description": "Should not match an empty log when message and severity are defined",
394+
"samplingConfig": {
395+
"logs": [
396+
{
397+
"message": {
398+
"regexValue": "Database connection .*"
399+
},
400+
"severityText": {
401+
"matchValue": "error"
402+
},
403+
"samplingRatio": 90
404+
}
405+
]
406+
},
407+
"inputLog": {},
408+
"samplerFunctionCases": [
409+
{
410+
"type": "always",
411+
"expected_result": {
412+
"sample": true
413+
}
414+
},
415+
{
416+
"type": "never",
417+
"expected_result": {
418+
"sample": true
419+
}
420+
}
421+
]
422+
},
423+
{
424+
"description": "Should not match a log with a message when message and severity are defined",
425+
"samplingConfig": {
426+
"logs": [
427+
{
428+
"message": {
429+
"regexValue": "Database connection .*"
430+
},
431+
"severityText": {
432+
"matchValue": "error"
433+
},
434+
"samplingRatio": 90
435+
}
436+
]
437+
},
438+
"inputLog": {
439+
"message": "Database connection failed: timeout"
440+
},
441+
"samplerFunctionCases": [
442+
{
443+
"type": "always",
444+
"expected_result": {
445+
"sample": true
446+
}
447+
},
448+
{
449+
"type": "never",
450+
"expected_result": {
451+
"sample": true
452+
}
453+
}
454+
]
455+
},
456+
{
457+
"description": "Should not match a log with only severity when message and severity are defined",
458+
"samplingConfig": {
459+
"logs": [
460+
{
461+
"message": {
462+
"regexValue": "Database connection .*"
463+
},
464+
"severityText": {
465+
"matchValue": "error"
466+
},
467+
"samplingRatio": 90
468+
}
469+
]
470+
},
471+
"inputLog": {
472+
"severityText": "error"
473+
},
474+
"samplerFunctionCases": [
475+
{
476+
"type": "always",
477+
"expected_result": {
478+
"sample": true
479+
}
480+
},
481+
{
482+
"type": "never",
483+
"expected_result": {
484+
"sample": true
485+
}
486+
}
487+
]
391488
}
392489
]

‎sdk/@launchdarkly/observability-shared/src/sampling/CustomSampler.test.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,14 @@ const runTestSpanScenarios = (scenarios: SpanTestScenario[]) => {
150150
})
151151
}
152152

153+
const toReadableLogRecord = (log: LogTestScenario['inputLog']) => {
154+
return {
155+
severityText: log.severityText,
156+
body: log.message,
157+
attributes: log.attributes,
158+
} as ReadableLogRecord
159+
}
160+
153161
const runTestLogScenarios = (scenarios: LogTestScenario[]) => {
154162
scenarios.forEach((scenario) => {
155163
scenario.samplerFunctionCases.forEach((samplerCase) => {
@@ -162,7 +170,7 @@ const runTestLogScenarios = (scenarios: LogTestScenario[]) => {
162170
expect(sampler.isSamplingEnabled()).toBe(true)
163171

164172
const result = sampler.sampleLog(
165-
scenario.inputLog as ReadableLogRecord,
173+
toReadableLogRecord(scenario.inputLog),
166174
)
167175

168176
expect(result.sample).toBe(samplerCase.expected_result.sample)

‎sdk/@launchdarkly/observability-shared/src/sampling/CustomSampler.ts‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -231,21 +231,17 @@ export class CustomSampler implements ExportSampler {
231231
record: ReadableLogRecord,
232232
): boolean {
233233
if (config.severityText) {
234-
const severityText = record.severityText
235-
if (
236-
typeof severityText === 'string' &&
237-
!this.matchesValue(config.severityText, severityText)
238-
) {
234+
if (!this.matchesValue(config.severityText, record.severityText)) {
239235
return false
240236
}
241237
}
242238

243239
if (config.message) {
244-
const message = record.body
245-
if (
246-
typeof message === 'string' &&
247-
!this.matchesValue(config.message, message)
248-
) {
240+
if (typeof record.body !== 'string') {
241+
// We only support string message bodies for now.
242+
return false
243+
}
244+
if (!this.matchesValue(config.message, record.body)) {
249245
return false
250246
}
251247
}

0 commit comments

Comments
 (0)