From b782e9ec62c041f4ea27a670057de698bb7280d6 Mon Sep 17 00:00:00 2001 From: Marc Lopez Rubio Date: Wed, 11 May 2022 15:39:00 +0800 Subject: [PATCH] processor: Avoid leaking labels between batches (#8081) Fixes a bug introduced in #6682 by performing a copy of the `baseEvent` before each of the events is decoded. The event was being shallow copied and it was reusing the same maps between batches. The linked PR changed the behavior (although not in a very obviously) and was instead merging the labels onto the passed `APMEvent`. This patch reintroduces the previous behavior in a more obvious manner. Signed-off-by: Marc Lopez Rubio Co-authored-by: stuart nelson --- ...TestPublishIntegrationEvents.approved.json | 17 ++------ ...PublishIntegrationMetricsets.approved.json | 12 ------ .../TestPublishIntegrationSpans.approved.json | 18 +++------ ...blishIntegrationTransactions.approved.json | 20 ++-------- changelogs/head.asciidoc | 1 + processor/stream/processor.go | 17 +++++++- processor/stream/processor_test.go | 39 +++++++++++++++++++ .../testIntakeIntegrationEvents.approved.json | 14 ++----- ...tIntakeIntegrationMetricsets.approved.json | 12 ------ ...tegrationOpenTelemetryBridge.approved.json | 9 ----- .../testIntakeIntegrationSpans.approved.json | 18 +++------ ...ntakeIntegrationTransactions.approved.json | 28 ++++--------- .../TestApprovedMetrics.approved.json | 12 ------ 13 files changed, 86 insertions(+), 131 deletions(-) diff --git a/beater/test_approved_es_documents/TestPublishIntegrationEvents.approved.json b/beater/test_approved_es_documents/TestPublishIntegrationEvents.approved.json index 0ae4cc0a733..901c890ef22 100644 --- a/beater/test_approved_es_documents/TestPublishIntegrationEvents.approved.json +++ b/beater/test_approved_es_documents/TestPublishIntegrationEvents.approved.json @@ -214,13 +214,10 @@ "labels": { "ab_testing": "true", "group": "experimental", - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "success": "true", - "wrapped_reporter": "true" + "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8" }, "message": "Request method 'POST' not supported", "numeric_labels": { - "code": 200, "segment": 5 }, "observer": { @@ -355,13 +352,9 @@ }, "labels": { "ab_testing": "true", - "group": "experimental", - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "success": "true", - "wrapped_reporter": "true" + "group": "experimental" }, "numeric_labels": { - "code": 200, "segment": 5 }, "observer": { @@ -558,11 +551,9 @@ "ab_testing": "true", "group": "experimental", "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "success": "true", "wrapped_reporter": "true" }, "numeric_labels": { - "code": 200, "segment": 5 }, "observer": { @@ -696,9 +687,7 @@ "labels": { "ab_testing": "true", "group": "experimental", - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "success": "true", - "wrapped_reporter": "true" + "success": "true" }, "metricset.name": "span_breakdown", "numeric_labels": { diff --git a/beater/test_approved_es_documents/TestPublishIntegrationMetricsets.approved.json b/beater/test_approved_es_documents/TestPublishIntegrationMetricsets.approved.json index 289b774b76a..58f755ad32a 100644 --- a/beater/test_approved_es_documents/TestPublishIntegrationMetricsets.approved.json +++ b/beater/test_approved_es_documents/TestPublishIntegrationMetricsets.approved.json @@ -86,13 +86,10 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": { @@ -141,13 +138,10 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": { @@ -198,13 +192,10 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": { @@ -267,8 +258,6 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "latency_distribution": { @@ -285,7 +274,6 @@ }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": { diff --git a/beater/test_approved_es_documents/TestPublishIntegrationSpans.approved.json b/beater/test_approved_es_documents/TestPublishIntegrationSpans.approved.json index 3432a37cb3e..ce1003470d2 100644 --- a/beater/test_approved_es_documents/TestPublishIntegrationSpans.approved.json +++ b/beater/test_approved_es_documents/TestPublishIntegrationSpans.approved.json @@ -69,8 +69,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -201,8 +200,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -466,8 +464,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -613,8 +610,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -819,8 +815,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -960,8 +955,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", diff --git a/beater/test_approved_es_documents/TestPublishIntegrationTransactions.approved.json b/beater/test_approved_es_documents/TestPublishIntegrationTransactions.approved.json index ad6f7866c16..dd3a5b741b2 100644 --- a/beater/test_approved_es_documents/TestPublishIntegrationTransactions.approved.json +++ b/beater/test_approved_es_documents/TestPublishIntegrationTransactions.approved.json @@ -64,14 +64,11 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", "tag1": "one", - "tag4": "false", "wrapped_reporter": "true" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -426,14 +423,11 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", "tag1": "one", - "tag4": "false", "wrapped_reporter": "true" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -579,14 +573,11 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", "tag1": "one", - "tag4": "false", "wrapped_reporter": "true" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", @@ -752,14 +743,11 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", "tag1": "one", - "tag4": "false", "wrapped_reporter": "true" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "observer": { "ephemeral_id": "00000000-0000-0000-0000-000000000000", diff --git a/changelogs/head.asciidoc b/changelogs/head.asciidoc index f21d3fb7ae3..bbde07587a0 100644 --- a/changelogs/head.asciidoc +++ b/changelogs/head.asciidoc @@ -11,6 +11,7 @@ https://github.com/elastic/apm-server/compare/8.2\...main[View commits] [float] ==== Bug fixes +- Fix a bug that caused some of the decoded events to have incorrect labels {pull}8081[8081] [float] ==== Intake API Changes diff --git a/processor/stream/processor.go b/processor/stream/processor.go index b4455114bc1..c18791e45f1 100644 --- a/processor/stream/processor.go +++ b/processor/stream/processor.go @@ -161,7 +161,9 @@ func (p *Processor) readBatch( // required for backwards compatibility - sending empty lines was permitted in previous versions continue } - input := modeldecoder.Input{Base: baseEvent} + // We copy the event for each iteration of the batch, as to avoid + // shallow copies of Labels and NumericLabels. + input := modeldecoder.Input{Base: copyEvent(baseEvent)} switch eventType := p.identifyEventType(body); string(eventType) { case errorEventType: err = v2.DecodeNestedError(reader, &input, batch) @@ -307,3 +309,16 @@ func (sr *streamReader) wrapError(err error) error { } return err } + +// copyEvent returns a shallow copy of the APMEvent with a deep copy of the +// labels and numeric labels. +func copyEvent(e model.APMEvent) model.APMEvent { + var out = e + if out.Labels != nil { + out.Labels = out.Labels.Clone() + } + if out.NumericLabels != nil { + out.NumericLabels = out.NumericLabels.Clone() + } + return out +} diff --git a/processor/stream/processor_test.go b/processor/stream/processor_test.go index c78328fa201..bf94ca258bc 100644 --- a/processor/stream/processor_test.go +++ b/processor/stream/processor_test.go @@ -24,6 +24,7 @@ import ( "io/ioutil" "net" "path/filepath" + "strings" "testing" "testing/iotest" "time" @@ -268,6 +269,44 @@ func TestRUMV3(t *testing.T) { } } +func TestLabelLeak(t *testing.T) { + payload := `{"metadata": {"service": {"name": "testsvc", "environment": "staging", "version": null, "agent": {"name": "python", "version": "6.9.1"}, "language": {"name": "python", "version": "3.10.4"}, "runtime": {"name": "CPython", "version": "3.10.4"}, "framework": {"name": "flask", "version": "2.1.1"}}, "process": {"pid": 2112739, "ppid": 2112738, "argv": ["/home/stuart/workspace/sdh/581/venv/lib/python3.10/site-packages/flask/__main__.py", "run"], "title": null}, "system": {"hostname": "slaptop", "architecture": "x86_64", "platform": "linux"}, "labels": {"ci_commit": "unknown", "numeric": 1}}} +{"transaction": {"id": "88dee29a6571b948", "trace_id": "ba7f5d18ac4c7f39d1ff070c79b2bea5", "name": "GET /withlabels", "type": "request", "duration": 1.6199999999999999, "result": "HTTP 2xx", "timestamp": 1652185276804681, "outcome": "success", "sampled": true, "span_count": {"started": 0, "dropped": 0}, "sample_rate": 1.0, "context": {"request": {"env": {"REMOTE_ADDR": "127.0.0.1", "SERVER_NAME": "127.0.0.1", "SERVER_PORT": "5000"}, "method": "GET", "socket": {"remote_address": "127.0.0.1"}, "cookies": {}, "headers": {"host": "localhost:5000", "user-agent": "curl/7.81.0", "accept": "*/*", "app-os": "Android", "content-type": "application/json; charset=utf-8", "content-length": "29"}, "url": {"full": "http://localhost:5000/withlabels?second_with_labels", "protocol": "http:", "hostname": "localhost", "pathname": "/withlabels", "port": "5000", "search": "?second_with_labels"}}, "response": {"status_code": 200, "headers": {"Content-Type": "application/json", "Content-Length": "14"}}, "tags": {"appOs": "Android", "email_set": "hello@hello.com", "time_set": 1652185276}}}} +{"transaction": {"id": "ba5c6d6c1ab44bd1", "trace_id": "88c0a00431531a80c5ca9a41fe115f41", "name": "GET /nolabels", "type": "request", "duration": 0.652, "result": "HTTP 2xx", "timestamp": 1652185278813952, "outcome": "success", "sampled": true, "span_count": {"started": 0, "dropped": 0}, "sample_rate": 1.0, "context": {"request": {"env": {"REMOTE_ADDR": "127.0.0.1", "SERVER_NAME": "127.0.0.1", "SERVER_PORT": "5000"}, "method": "GET", "socket": {"remote_address": "127.0.0.1"}, "cookies": {}, "headers": {"host": "localhost:5000", "user-agent": "curl/7.81.0", "accept": "*/*"}, "url": {"full": "http://localhost:5000/nolabels?third_no_label", "protocol": "http:", "hostname": "localhost", "pathname": "/nolabels", "port": "5000", "search": "?third_no_label"}}, "response": {"status_code": 200, "headers": {"Content-Type": "text/html; charset=utf-8", "Content-Length": "14"}}, "tags": {}}}}` + + baseEvent := model.APMEvent{ + Host: model.Host{IP: []net.IP{net.ParseIP("192.0.0.1")}}, + } + + var processed *model.Batch + batchProcessor := model.ProcessBatchFunc(func(_ context.Context, b *model.Batch) error { + processed = b + return nil + }) + + p := BackendProcessor(&config.Config{MaxEventSize: 100 * 1024}, make(chan struct{}, 1)) + var actualResult Result + err := p.HandleStream(context.Background(), baseEvent, strings.NewReader(payload), 10, batchProcessor, &actualResult) + require.NoError(t, err) + + txs := *processed + assert.Len(t, txs, 2) + // Assert first tx + assert.Equal(t, model.NumericLabels{ + "time_set": {Value: 1652185276}, + "numeric": {Value: 1}, + }, txs[0].NumericLabels) + assert.Equal(t, model.Labels{ + "appOs": {Value: "Android"}, + "email_set": {Value: "hello@hello.com"}, + "ci_commit": {Value: "unknown"}, + }, txs[0].Labels) + + // Assert second tx + assert.Equal(t, model.NumericLabels{"numeric": {Value: 1}}, txs[1].NumericLabels) + assert.Equal(t, model.Labels{"ci_commit": {Value: "unknown"}}, txs[1].Labels) +} + func makeApproveEventsBatchProcessor(t *testing.T, name string, count *int) model.BatchProcessor { return model.ProcessBatchFunc(func(ctx context.Context, b *model.Batch) error { events := b.Transform(ctx) diff --git a/processor/stream/test_approved_es_documents/testIntakeIntegrationEvents.approved.json b/processor/stream/test_approved_es_documents/testIntakeIntegrationEvents.approved.json index 59cb0c95a1c..932074d0ec9 100644 --- a/processor/stream/test_approved_es_documents/testIntakeIntegrationEvents.approved.json +++ b/processor/stream/test_approved_es_documents/testIntakeIntegrationEvents.approved.json @@ -207,11 +207,9 @@ "labels": { "ab_testing": "true", "group": "experimental", - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "success": "true" + "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8" }, "numeric_labels": { - "code": 200, "segment": 5 }, "parent": { @@ -334,12 +332,9 @@ }, "labels": { "ab_testing": "true", - "group": "experimental", - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "success": "true" + "group": "experimental" }, "numeric_labels": { - "code": 200, "segment": 5 }, "parent": { @@ -523,11 +518,9 @@ "labels": { "ab_testing": "true", "group": "experimental", - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "success": "true" + "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8" }, "numeric_labels": { - "code": 200, "segment": 5 }, "parent": { @@ -649,7 +642,6 @@ "labels": { "ab_testing": "true", "group": "experimental", - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", "success": "true" }, "numeric_labels": { diff --git a/processor/stream/test_approved_es_documents/testIntakeIntegrationMetricsets.approved.json b/processor/stream/test_approved_es_documents/testIntakeIntegrationMetricsets.approved.json index 613cd96841e..831153ffa95 100644 --- a/processor/stream/test_approved_es_documents/testIntakeIntegrationMetricsets.approved.json +++ b/processor/stream/test_approved_es_documents/testIntakeIntegrationMetricsets.approved.json @@ -67,12 +67,9 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "numeric_labels": { - "code": 200, "tag2": 2 }, "process": { @@ -109,12 +106,9 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "numeric_labels": { - "code": 200, "tag2": 2 }, "process": { @@ -153,12 +147,9 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "numeric_labels": { - "code": 200, "tag2": 2 }, "process": { @@ -209,8 +200,6 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "latency_distribution": { @@ -226,7 +215,6 @@ ] }, "numeric_labels": { - "code": 200, "tag2": 2 }, "process": { diff --git a/processor/stream/test_approved_es_documents/testIntakeIntegrationOpenTelemetryBridge.approved.json b/processor/stream/test_approved_es_documents/testIntakeIntegrationOpenTelemetryBridge.approved.json index 8db896218ea..34cd0676c07 100644 --- a/processor/stream/test_approved_es_documents/testIntakeIntegrationOpenTelemetryBridge.approved.json +++ b/processor/stream/test_approved_es_documents/testIntakeIntegrationOpenTelemetryBridge.approved.json @@ -64,7 +64,6 @@ "tag1": "one" }, "numeric_labels": { - "double_value": 123.456, "int_array": [ 1, 2 @@ -200,18 +199,10 @@ } }, "labels": { - "string_array": [ - "a", - "b" - ], "tag1": "one" }, "numeric_labels": { "double_value": 123.456, - "int_array": [ - 1, - 2 - ], "tag2": 2 }, "parent": { diff --git a/processor/stream/test_approved_es_documents/testIntakeIntegrationSpans.approved.json b/processor/stream/test_approved_es_documents/testIntakeIntegrationSpans.approved.json index 882c20bdd57..1fad2e20ca4 100644 --- a/processor/stream/test_approved_es_documents/testIntakeIntegrationSpans.approved.json +++ b/processor/stream/test_approved_es_documents/testIntakeIntegrationSpans.approved.json @@ -62,8 +62,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "parent": { "id": "abcdef0123456789" @@ -178,8 +177,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "parent": { "id": "0000000011111111" @@ -411,8 +409,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "parent": { "id": "ababcdcdefefabde" @@ -542,8 +539,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "parent": { "id": "abcdef0123456789" @@ -732,8 +728,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "parent": { "id": "abcdef0123456789" @@ -857,8 +852,7 @@ } }, "labels": { - "tag1": "value1", - "tag4": "true" + "tag1": "label1" }, "parent": { "id": "abcdef0123456789" diff --git a/processor/stream/test_approved_es_documents/testIntakeIntegrationTransactions.approved.json b/processor/stream/test_approved_es_documents/testIntakeIntegrationTransactions.approved.json index 87b719cd290..cbe1ed05167 100644 --- a/processor/stream/test_approved_es_documents/testIntakeIntegrationTransactions.approved.json +++ b/processor/stream/test_approved_es_documents/testIntakeIntegrationTransactions.approved.json @@ -57,13 +57,10 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "tag1": "one", - "tag4": "false" + "tag1": "one" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "parent": { "id": "abcdefabcdef01234567" @@ -391,13 +388,10 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "tag1": "one", - "tag4": "false" + "tag1": "one" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "process": { "args": [ @@ -530,13 +524,10 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "tag1": "one", - "tag4": "false" + "tag1": "one" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "parent": { "id": "abcdefabcdef01234567" @@ -689,13 +680,10 @@ } }, "labels": { - "organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8", - "tag1": "one", - "tag4": "false" + "tag1": "one" }, "numeric_labels": { - "tag2": 12, - "tag3": 12.45 + "tag2": 2 }, "process": { "args": [ diff --git a/systemtest/approvals/TestApprovedMetrics.approved.json b/systemtest/approvals/TestApprovedMetrics.approved.json index ac8dfd73d79..a3859b3264c 100644 --- a/systemtest/approvals/TestApprovedMetrics.approved.json +++ b/systemtest/approvals/TestApprovedMetrics.approved.json @@ -23,13 +23,10 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": { @@ -83,13 +80,10 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": { @@ -145,13 +139,10 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": { @@ -213,8 +204,6 @@ ] }, "labels": { - "some": "abc", - "success": "true", "tag1": "one" }, "latency_distribution": { @@ -231,7 +220,6 @@ }, "metricset.name": "app", "numeric_labels": { - "code": 200, "tag2": 2 }, "observer": {