[Monitoring] De-duplicate pipeline ids based on the ephemeral_id changing

[chrisronline]

Resolves #49462

This PR fixes an issue where users saw duplicate pipeline ids in their pipeline listing page because they had restarted their pipelines (which causes the ephemeral_id to change).

In our logic to get the entire list of pipeline ids, we are doing a composite agg across id, hash and ephemeral_id because we need those lists in order to reduce the number of buckets created in the subsequent queries (see here for more information). If the ephemeral_id changes over time for a given pipeline, two results will show for the single id resulting in the behavior described in #49462

To reproduce, simply setup logstash with multiple pipelines and enable monitoring. Navigate to the pipelines listing page to verify they show up there, then restart logstash. Before this fix, duplicates should show up.

Notes

• I'm fairly sure it isn't likely we show incorrect data - by de-duplicating, we're only fetching data for each id once, even if there are multiple ephemeral_id ids for that id. Maybe @cachedout can help educate me a bit here

TODO

• Add tests

[elasticmachine]

Pinging @elastic/stack-monitoring (Team:Monitoring)

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 97e67f5

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ef35e5f

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 399b469

[amitavmohanty01]

@chrisronline Can you please merge the fix to 7.4 branch as well? As of now, 7.4 is the current branch as per documentation so I would request you to release this fix for 7.4 as well.

[igoristic]

Great job! Works as expected 👍

Thanks for the instructions on reproducing the behavior

Approved so long as @cachedout addresses your concerns in the description and merge upstream passes

[cachedout]

This could work but it has an important caveat that I'll describe below. I think that what we may want here is actually to ensure that the hash key is the one that's unique versus using id for this purpose. I'll demonstrate below why I think this may be the case.

Scenario 1

A simple pipeline definition

config.pipelines.yml

 - pipeline.id: test
   pipeline.workers: 1
   pipeline.batch.size: 1
   config.string: "input { generator {} } filter { sleep { time => 1 } } output { stdout { codec => dots } }"

Below is the JSON output for two keys:

[Filter]> .pipelines.test.ephemeral_id
"badcbe9e-c4e2-46d1-8689-8f07af3c4f85"

[Filter]> .pipelines.test.hash
"a3bf686e789a4fbb312192cf2f27667831f9c56b7c9394c1f2d8b933fe5c3ddc"

Scenario 2

Using the same pipeline definition as Scenario 1 and simply restarting Logstash after collecting data from Scenario 1

[Filter]> .pipelines.test.ephemeral_id
"bc92413c-556d-4ce3-b0ff-eba23406821a"

[Filter]> .pipelines.test.hash
"a3bf686e789a4fbb312192cf2f27667831f9c56b7c9394c1f2d8b933fe5c3ddc"

As we can see, the ephemeral identifier has changed but the hash (and the ID which is not shown above) both remain the same.

Scenario 3

Changing the pipeline definition and restarting Logstash

Here we change the pipeline definition by altering the sleep timer to 2 seconds.

 - pipeline.id: test
   pipeline.workers: 1
   pipeline.batch.size: 1
   config.string: "input { generator {} } filter { sleep { time => 2 } } output { stdout { codec => dots } }"

[Filter]> .pipelines.test.ephemeral_id
"cd880b73-6dcd-4678-9459-61f70bc843be"

[Filter]> .pipelines.test.hash
"5fd63e8270d1cdae04a7ebdb95626dc93f40e4d1355df2044a70825a4ef70e10"

Summary

In every scenario, the pipeline identifier is the same and is returned as test. When Logstash restarts, we see the ephemeral identifier change as expected while the hash remains the same -- indicating that the pipeline definition has not changed; e.g. it is the same pipeline.

However, when the pipeline definition is modified it becomes an ostensibly different pipeline, despite having the same ID as a previously defined pipeline. As such, we may wish to consider it as a different entity for the purposes of monitoring.

The counterpoint, of course, is that this may be surprising to the user especially if they make many changes to the same pipeline definition.

So, it may be that using the ID is the right thing to do from the perspective of the user who might be more inclined to believe that they're working with the same pipeline over time even though Logstash sees these internally as different, but I think this is worthy of a further discussion. I'll leave it there and wait for comments before we come to a decision. :)

[cachedout]

I left my review notes in comment on the PR itself.

[chrisronline]

@chrisronline Can you please merge the fix to 7.4 branch as well? As of now, 7.4 is the current branch as per documentation so I would request you to release this fix for 7.4 as well.

Sure!

[amitavmohanty01]

@cachedout As a user, the following matters to me.

When a pipeline is defined as follows:

- pipeline.id: syslog
  path.config: "/etc/logstash/syslog.conf"

The changes to the pipeline are done in the conf file. From a monitoring perspective, the pipeline should be the same after every restart. This functionality allows me to compare the effect of the changes on the performance of the pipeline.

Taking a step back, I would like to pose the following situation.

** Box: 1 **

- pipeline.id: syslog
  path.config: "/etc/logstash/syslog.conf"

** Box: 2 **

- pipeline.id: syslog
  path.config: "/etc/logstash/syslog_other.conf"

The error above can be deliberate or it can also be a typo by the user.

From a design perspective, one option is to say, all pipelines with the same id will be considered together in the aggregation on the monitoring page. It is an easier to implement option and puts the onus of maintaining correct config on the user. Another option is to aggregate on id as long as the config matches. It would require deep comparision of each pipeline. Yet another option would be not let the option of having such a config as shown above. In that case, monitoring page should throw an error when loaded. This however rules out the cases when the user is creating such a config deliberately.

[chrisronline]

Thanks all for the thoughts!

I'm fairly sure it isn't likely we show incorrect data - by de-duplicating, we're only fetching data for each id once, even if there are multiple ephemeral_id ids for that id. Maybe @cachedout can help educate me a bit here

I ran some tests locally and I don't think this has much merit. For the listing page (versus the actual pipeline page), we are showing aggregated data from the last two buckets in the sequence (two so we can perform any necessary derivatives). It's unlikely that the hash/ephemeral_id change so frequently that in a 20s span, there is much of a difference in calculated metrics.

Assuming everyone is okay with it, I think we ignore the concern and move on with the PR.

[chrisronline]

@elasticmachine merge upstream

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 414cdfa

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: aaf727a

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: aa0d641

[chrisronline]

Backport:

7.4: e1cccc8
7.5: f2e71f4
7.x: fca66a0