[Metricbeat] Stop continual error with multiple Logstash pipelines

[Kerry350]

What does this PR do?

This PR closes #31739.

This PR stops multiple errors being logged when using Logstash with multiple pipelines.

⚠️ Please see the fields / mapping inconsistency section I've added in this description, because whilst this PR fixes the logging issue, there are some questions. ⚠️

Why is it important?

The Logs are noisy.

Checklist

• My code follows the style guidelines of this project (I think so, couldn't find a STYLEGUIDE file)
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

How to test this PR locally

• Follow the docs to setup your environment to build Beats (here: https://www.elastic.co/guide/en/beats/devguide/current/beats-contributing.html#setting-up-dev-environment)

• Checkout this branch, and thencd metricbeat, and then mage build.

• Make a Metricbeat config file that contains the following (change the credentials):

http.enabled: true
metricbeat.modules:
  - module: system

  - module: logstash
    xpack.enabled: true
    period: 10s
    hosts: [ "localhost:9600" ]

output.elasticsearch:
  hosts: [ "localhost:9200" ]
  username: "elastic"
  password: "changeme"

• Then run ./metricbeat -e -c PATH_TO_YOUR_METRICBEAT_YML_FILE

• Run Logstash with multiple pipelines.

docker run --name logstash \
  --pull always --rm \
  --hostname=logstash \
  --publish=9600:9600 \
  --volume="$(pwd)/x-pack/plugins/monitoring/dev_docs/reference/logstash.yml:/usr/share/logstash/config/logstash.yml:ro" \
  --volume="$(pwd)/x-pack/plugins/monitoring/dev_docs/reference/pipelines.yml:/usr/share/logstash/config/pipelines.yml:ro" \
  docker.elastic.co/logstash/logstash:master-SNAPSHOT

You should not see excessive logging, e.g.:

Related issues

Closes #31739.

Logs

Excessive logging before:

Fields / mapping inconsistency

It is noted in the issue that:

The deletions seem to have been added in #10350 presumably to avoid leaving fields in the document that don't comply with ECS

and this seemed like a very reasonable assumption. As such I originally moved

event.MetricSetFields.Update(fields)

below

if err = commonFieldsMapping(&event, fields); err != nil {
  return err
}

because it's commonFieldsMapping that calls fields.Delete(). This means things like host and version wouldn't end up on logstash.node. This also aligned with the documentation. We can see there that logstash.node.host and co are supposed to be an alias. However, that's not the way the mappings seem to work. These are not aliased. So having event.MetricSetFields.Update(fields) copy everything (the full set of fields) seems necessary. It doesn't align with the docs, but it aligns with the mappings (and most likely how solutions are accessing these fields). There definitely seems to be confusion here in expectation, unless I'm misunderstanding.

I have also added a Gist here with results produced by all three. Before changes, after changes with field deletion, and after changes with no field deletion. We can see that in terms of indexed data before and after changes with no field deletion match.

As such, this PR just fixes the logging problem by making a new fields map each time, which aligns with this loop example.

I am not sure how we want to approach the fields and mappings. It seems that we should update the mappings to accurately reflect the docs regarding aliases, and id, host, version should no longer exist on logstash.node.

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-06-17T17:57:53.237+0000

• Duration: 62 min 46 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3541
Skipped	887
Total	4428

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

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

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

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

[klacabane]

It seems that we should update the mappings to accurately reflect the docs regarding aliases, and id, host, version should no longer exist on logstash.node.

Agreed. looks like the aliases were lost in translation when creating the logstash integrations (which the .monitoring-{product}-mb mappings are based on) because they were defined in the 7.x version

[Kerry350]

Agreed. looks like the aliases were lost in translation when creating the logstash integrations (which the .monitoring-{product}-mb mappings are based on) because they were defined in the 7.x version

Gotcha, makes sense 👍

I'll merge this since it's part of the GA work. And file an issue for the mappings / aliases.