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: Avoid leaking labels between batches #8081

Merged
merged 5 commits into from
May 11, 2022

Conversation

marclop
Copy link
Contributor

@marclop marclop commented May 10, 2022

Motivation/summary

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.

Checklist

How to test these changes

  1. docker-compose up -d and install the APM Integration
  2. make && ./apm-server -E output.elasticsearch.username=admin -E output.elasticsearch.password=changeme -E apm-server.expvar.enabled=true -e
  3. curl -H "Content-type: application/x-ndjson" --data-binary @testdata/intake-v2/events.ndjson http://localhost:8200/intake/v2/events
  4. Navigate to Kibana dev tools and issue the following query:
GET traces*/_search
{
  "_source": false, 
  "fields": [
    "labels.*",
    "numeric_labels.*"
  ], 
  "query": {
    "match": {
      "labels.organization_uuid": "9f0e9d64-c185-4d21-a6f4-4673ed561ec8"
    }
  }
}
  1. Verify there's only 1 hit and it's:
{
    "_index": ".ds-traces-apm-default-2022.05.10-000001",
    "_id": "joj0rYABBkOJH5O9XGQe",
    "_score": 0.2876821,
    "fields": {
        "labels.group": [
            "experimental"
        ],
        "labels.ab_testing": [
            "true"
        ],
        "numeric_labels.segment": [
            5
        ],
        "labels.organization_uuid": [
            "9f0e9d64-c185-4d21-a6f4-4673ed561ec8"
        ]
    }
}

Related issues

Fixes a bug introduced in elastic#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.

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@marclop marclop added bug v8.3.0 backport-8.2 Automated backport with mergify labels May 10, 2022
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@marclop marclop requested a review from a team May 10, 2022 12:41
@apmmachine
Copy link
Contributor

apmmachine commented May 10, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-05-11T07:12:38.054+0000

  • Duration: 26 min 12 sec

Test stats 🧪

Test Results
Failed 0
Passed 3993
Skipped 13
Total 4006

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /hey-apm : Run the hey-apm benchmark.

  • /package : Generate and publish the docker images.

  • /test windows : Build & tests on Windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@apmmachine
Copy link
Contributor

apmmachine commented May 10, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (43/43) 💚
Files 91.919% (182/198) 👍
Classes 93.421% (426/456) 👍
Methods 89.309% (1086/1216) 👍 0.009
Lines 76.866% (13181/17148) 👍 0.012
Conditionals 100.0% (0/0) 💚

- remove unnecessary channel waitc
- add numeric label in metadata, also. this
  triggers a leak in the following events'
  NumericLabels
Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

The explanation for why we need this makes sense, as does why this is only seen when metadata labels exist (the apmevent that is getting copied now has a shared map that doesn't get reset). I still don't know how it worked before, but I'll see if I can make sense of it.

@stuartnelson3
Copy link
Contributor

We used to clone the labels when merging, which got removed: https://github.com/elastic/apm-server/pull/6682/files#diff-b3024df769ff9c30ace55348c1326f0cfd0bcc32e819705a2f51255f50ccb9a1L56

This PR is re-introducing that behavior, but in a more obvious way.

@stuartnelson3
Copy link
Contributor

/test

@marclop marclop enabled auto-merge (squash) May 11, 2022 07:12
@marclop marclop merged commit b782e9e into elastic:main May 11, 2022
@marclop marclop deleted the b/fix-cross-batch-event-copying branch May 11, 2022 07:39
mergify bot pushed a commit that referenced this pull request May 11, 2022
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 <marc5.12@outlook.com>
Co-authored-by: stuart nelson <stuartnelson3@gmail.com>
(cherry picked from commit b782e9e)

# Conflicts:
#	changelogs/head.asciidoc
stuartnelson3 added a commit that referenced this pull request May 11, 2022
#8086)

* 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 <marc5.12@outlook.com>
Co-authored-by: stuart nelson <stuartnelson3@gmail.com>
(cherry picked from commit b782e9e)

# Conflicts:
#	changelogs/head.asciidoc

* Delete head.asciidoc

Co-authored-by: Marc Lopez Rubio <marc5.12@outlook.com>
Co-authored-by: stuart nelson <stuartnelson3@gmail.com>
@marclop
Copy link
Contributor Author

marclop commented May 24, 2022

Verified with the reproduction steps in 8.2.1 BC2:

{
  "took" : 7,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 1,
      "relation" : "eq"
    },
    "max_score" : 0.2876821,
    "hits" : [
      {
        "_index" : ".ds-traces-apm-default-2022.05.24-000001",
        "_id" : "cVo19YAByBhr8ADC4Z26",
        "_score" : 0.2876821,
        "fields" : {
          "labels.group" : [
            "experimental"
          ],
          "labels.ab_testing" : [
            "true"
          ],
          "numeric_labels.segment" : [
            5.0
          ],
          "labels.organization_uuid" : [
            "9f0e9d64-c185-4d21-a6f4-4673ed561ec8"
          ]
        }
      }
    ]
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants