Apache ingest pipeline doesn't tolerate existing "event.original" field

[jsvd]

From the apache ingest pipeline (link):

  - rename:
      field: message
      target_field: event.original

The rename processor documentation states:

If the field doesn’t exist or the new name is already used, an exception will be thrown.

This means that any document that already has an "event.original" field (with or without a "message") field will cause an ingestion error:

"message": "......",
"error": {
"message": "field [event.original] already exists"
},

A suggestion is to tolerate the presence of "event.original" and "message" fields by including an if condition in the rename processor.

[lalit-satapathy]

Hi @jsvd,

Is the issue only for "apache" integration. What happens for the other integrations which also renames "message" to "event.original"?

[ishleenk17]

@lalit-satapathy : This issue will be for integrations which are using rename but are not using either of the below:

1. ignore_failure: true
2. if ctx.event.original == null check

[ishleenk17]

LS will be resolving this issue with : https://github.com/logstash-plugins/logstash-input-elastic_agent/issues/3

[jsvd]

Following up here: Logstash will work on ensuring the Logstash's elastic_agent input doesn't modify data coming from Elastic Agent.
That said we'd still like to encourage ingest pipelines to be more tolerant to existing event.original fields and overall other rename operations. Any thoughts on proceeding with #3464?

[jsvd]

Some quick greps show that there are currently 213 pipelines with a rename processor that moves "message" to "event.original":

~/elastic/integrations
❯ grep -r -A 3 "rename:" . | grep -A 2 " field: message" | grep  "target_field: event.original" |  wc -l
     213

More than half of these skip the rename if the "message" field exists through the use of ignore_missing: true.

~/elastic/integrations main
❯ grep -r -A 3 "rename:" . | grep -A 2 " field: message" | grep -A 2 "target_field: event.original" | grep "ignore_missing: true" | wc -l
     132

Having only the ignore_missing enabled allows pipelines to receive data with just the "event.original", but will still crash with both fields exist.

A few already are protected against the existence of "event.original" through the use of a conditional checking if "event.original" isn't null:

❯ grep -r -A 5 "rename:" . | grep -A 3 " field: message" | grep -A 3 "target_field: event.original" | grep "if.*event.*original.*null"  | wc -l
       8

@ishleenk17 do you think it's worth broadening the scope of this issue or create a new one to standardize how integrations pipelines handle "message" and "event.original"?

[philippkahr]

We really need to find a way to have a standardised setting. We ran into this using Logstash, which adds the event.original field since 8. Thus making the Fortinet pipeline unusable as it does not allow failure for the set event.original processor.

Maybe we can find a way to leverage some form of tests that make sure that a skeleton of the integrations looks the same? This also goes a long way to the settings that every integration should offer, such as tags, processors...

[GalchukRuth]

Hi,
Struggling with the same problem in AWS module.
We see in logs field message and also event.original.
With the same error: "field [event.original] already exists"

Filebeat version 8.5

When will the problem be fixed?

[ishleenk17]

@jsvd can share the ETA for https://github.com/logstash-plugins/logstash-input-elastic_agent/issues/3. this looks to be important to avoid any further SDH's.

[jsvd]

hi folks, on our side we're close to merging logstash-plugins/logstash-input-beats#464 (comment) which will allow disabling several kinds of enrichments, including the event.original.

[jsvd]

Starting with Logstash 8.7.0, the Agent/Beats inputs now support an enrich => setting so that data taken in as is from Beats. More on this setting here: https://www.elastic.co/guide/en/logstash/current/plugins-inputs-beats.html#plugins-inputs-beats-enrich. An example configuration, taken from the documentation:

input {
  beats {
    port => 5044
    enrich => none
  }
}

This closes the loop on the Logstash side, I believe there's still a goal to resolve the inconsistent message/event.original handling across integrations hinted at in #3451 (comment)

[lucabelluccini]

I am confused by using enrich, while we had logstash-plugins/logstash-codec-json#43 attempting to address the same issue.

Can Logstash, Ingest team and Integrations developers sync up in order to document what users should do to make data flow from Elastic Agents to Logstash to Elasticsearch properly?
Or if we got to a clear solution, document it in https://www.elastic.co/guide/en/fleet/current/logstash-output.html ?

[roaksoax]

Logstash has always added metadata to the event it receives, and in turn (due to ECS), creates event.original. The enrich option allows users to decide whether they want Logstash to continue adding this metadata (or which metadata for the matter), or whether they would like Logstash to passthrough the data untouched. This is so regardless of Agent Integrations.

While this was originally added to help with the lack of consistency in message/event.original by Integrations, users may want to continue to be able to have the metadata added by Logstash, and as such, Integrations should be consistent and be able to deal with the fact that event.original is present.

[philippkahr]

@joshdover can you track this to make sure that all ingest pipeline can deal with an already existing event.original?

[joshdover]

@mrodm What do you think the best way to handle this across all integrations would be? I think we could add a linting/validation rule to package-spec to avoid a rename processor without checking if event.original already exists.

I'm not 100% sure we can catch all cases this way, but it would be a good place to start. A foolproof option would be a test case that attempts to ingest data with a non-empty event.original and verify there are no pipeline errors. This seem expensive to run and probably not worth the effort right now?

[mrodm]

@mrodm What do you think the best way to handle this across all integrations would be? I think we could add a linting/validation rule to package-spec to avoid a rename processor without checking if event.original already exists.

@josh adding this new validation rule or lint to the package-spec would be a breaking change. It would make all the packages that do not fit that to fail.
If that path is followed, maybe it would be better to check in that new validation rule that these two conditions are fulfilled:

• a rename processor with those specific field and target_field parameters exist in the pipeline and it contains the expected if or ignore_missing fields:

    - rename:
        field: message
        target_field: event.original
        # ignore_missing: true
        # if: 'ctx.event?.original == null'

• a remove processor is also added to remove the "message" field, to avoid duplication.

    - remove:
        field: message
        ignore_missing: true
        if: 'ctx.event?.original != null'

So, something like it has been done in this PR: #7026

A foolproof option would be a test case that attempts to ingest data with a non-empty event.original and verify there are no pipeline errors. This seem expensive to run and probably not worth the effort right now?

Another option, it would be adding "logstash" to the stack (somehow optional). This should be optional since we should keep testing packages without logstash. Doing that, it could be checked how packages behave with that event.original field present. WDYT ?

@elastic/ecosystem please chime in here if I miss anything

[joshdover]

@mrodm Let's go with the first option. Can you open the appropriate package-spec issues?

[mrodm]

@josh sure! issues created

• new validation rule in package-spec: Add new validation rule to avoid rename processor without checking event.original package-spec#583
• add a new logstash service in elastic-package optionally: Add logstash as a component in the stack boot up by elastic-package  elastic-package#1432

Thinking about the new validation rule in package-spec, should that issue be added as part of the Package Spec V3 elastic/package-spec#539 ? This is likely to introduce a breaking change for existing packages. @joshdover @jsoriano

[jlind23]

@joshdover I will plan the issues that Mario mentioned above. What should we do with this specific integration's issue?

[joshdover]

Thanks, @jlind23.

@ishleenk17 Are you still planning to work on this? How should we get this prioritized on the Infra Obs team?

[ishleenk17]

@joshdover: I went through the above discussions. I am trying to articulate my understanding regarding the 3 aspects of the solution:

Logstash

At the Logstash side, there is an enrich flag, which is disabled by default, hence not making any changes to the data coming in from Beats (Solving the actual issue).
In case, enrich is enabled: Metadata will be added by logstash and the actual issue might be seen again.
This change is already done

Integrations

To fix the above-discussed issue(when enrich is enabled), changes are needed in the Integrations, where we have to add both of the 2 checks (if check and ignore-missing check) to the rename processors in the ingest pipelines.
This change is yet to be done.

Elastic-Package

To ensure that there is no rename processor without either of these 2 checks, package-spec changes are being done as well, where a developer will get an error if the rename processor misses a check.
This change is yet to be done.

Is my understanding correct?
cc: @lalit-satapathy @rameshelastic

[joshdover]

Thanks @ishleenk17. That matches my understanding, thanks for putting the pieces together 🕵️

[lalit-satapathy]

To fix the above-discussed issue(when enrich is enabled), changes are needed in the Integrations, where we have to add both of the 2 checks (if check and ignore-missing check) to the rename processors in the ingest pipelines.
This change is yet to be done.

If we need both these checks, we may end of changing most of the existing ingest pipeline for this change.

adding this new validation rule or lint to the package-spec would be a breaking change. It would make all the packages that do not fit that to fail.

Will this check, when added fail existing packages, which will force to be updated?

[lalit-satapathy]

@ishleenk17,

In which case, does a data stream requires a rename processor for message to event.original? Is this applicable for all log data streams?

If most data streams will need to add such a rename processor for message to event.original, and too, in the same format (with ignore_missing and if check), can we explore an option to add them into, a default ingest pipeline?

[ishleenk17]

@ishleenk17,

In which case, does a data stream requires a rename processor for message to event.original? Is this applicable for all log data streams?

If most data streams will need to add such a rename processor for message to event.original, and too, in the same format (with ignore_missing and if check), can we explore an option to add them into, a default ingest pipeline?

Most of the log datastreams do use the rename processor from message to event.original as the grok pattern is then applied on top of event.original.
But there are some integrations, where rename is not used and groking is done directly on the message. Eg: Mongodb

In case we make it the default option, we need to handle these scenarios.
Also, it would be good to know examples of such processors that have been made default in the pipelines before.

[philippkahr]

How will any change interfere with the preserve event.original toggle?

[ishleenk17]

How will any change interfere with the preserve event.original toggle?

Preserve original event will just not remove the event.original field in the end.
Adding if checks to the renaming of message to event.original should not impact preserve original functionality.

@P1llus : do you have other thoughts on this since you implemented the preserve original event?

[philippkahr]

The only thing that is mandatory in my view is, that the preserve original event, needs to happen at the earliest stage (the first processor in the ingest pipeline), like with the rename processor on various other integrations already. If e.g. mongoDB does not have a rename message to event.original at all, how does it then work when I hit the preserve original event?

[ishleenk17]

The only thing that is mandatory in my view is, that the preserve original event, needs to happen at the earliest stage (the first processor in the ingest pipeline), like with the rename processor on various other integrations already. If e.g. mongoDB does not have a rename message to event.original at all, how does it then work when I hit the preserve original event?

The processor looks like this that checks for preserve original event

- remove:
    field: event.original
    if: "ctx?.tags == null || !(ctx.tags.contains('preserve_original_event'))"
    ignore_failure: true
    ignore_missing: true

If event.original is not present, then the ignore_missing flag will just skip the processor.

[philippkahr]

But how does this work in the MongoDB case, when there is no rename: message to event.original?

As long as this toggle exists, doesn't that make it mandatory that every integration has the rename message to event.original processor at the very top of it's processing?

[P1llus]

@joshdover @ishleenk17 @philippkahr There is a few pointers here that needs to be considered, and we should provide a short and long term solution.

1. I feel logstash should not be creating event.original when the data comes from an integration, that should be fairly doable to add as a check in Logstash? The reason is that I am unsure if the format of event.original is always the same when it is created from logstash rather than our ingest pipeline?
2. The rename processor should indeed be at the top, and we should ensure they both have ignore_missing and a check that event.original does not already exist (this is the short term solution, as it does not require a stack upgrade, but is dependant on the fact that Logstash provides the same format we expect.
3. Any integration that do not do this (like mongodb) needs to have this added, this should be considered a bug.
4. The remove processor for event.original has an issue open to be moved to the final_pipeline rather than being defined in each integration, that makes it easier to do this.

Some more long term fixes/possibilities that I see we should consider:

1. We should add a flag somewhere, either on each libbeat/filebeat input, the outputs, or the beats processors that agent injects into beats that renames the message field to event.original even before it leaves the beat. I rather have it on the input itself to ensure its actually the original event (and not a modified one in some way).
2. That way logstash can check for the field existing already, making it easier in the long term.
3. The field mapping for event.original creates certain issues for synthetic source etc, maybe we could look into not providing the field mapping unless the feature is actually enabled?

[ishleenk17]

1. I feel logstash should not be creating event.original when the data comes from an integration, that should be fairly doable to add as a check in Logstash? The reason is that I am unsure if the format of event.original is always the same when it is created from logstash rather than our ingest pipeline?

Logstash has already made the chnage by adding the enrich flag and taking action accordingly. #3451 (comment)

• The rename processor should indeed be at the top, and we should ensure they both have ignore_missing and a check that event.original does not already exist (this is the short term solution, as it does not require a stack upgrade, but is dependant on the fact that Logstash provides the same format we expect.

Yes, this is what we need to do now for all Integrations missing this check.

1. We should add a flag somewhere, either on each libbeat/filebeat input, the outputs, or the beats processors that agent injects into beats that renames the message field to event.original even before it leaves the beat. I rather have it on the input itself to ensure its actually the original event (and not a modified one in some way).

Correct, we can have it as a long term plan

[joshdover]

The remove processor for event.original has an issue open to be moved to the final_pipeline rather than being defined in each integration, that makes it easier to do this.

I've added a note to an open PR that also updates the final pipeline to include this new processor too: elastic/kibana#167318