feat(outputs): Only copy metric if its not filtered out

[LarsStegman]

Summary

This PR makes sure that an output plugin will really select a metric for outputting, before copying it. This improvement had a big impact on runtime/gc time in real world performance. After this change the amount of gc time went down from 55% of the CPU time to 25%.

In our real world case every output plugin was only interested in a subset of the metric and there was no overlap between the subsets. This means that (x-1)/x% of all the copied metrics were immediately discarded, where x is the number of outputs.

Checklist

• No AI generated code was used in this PR

Related issues

resolves #15882

[srebhan]

Thanks a lot @LarsStegman for your investigation!!!

How about moving the copy into the running_output model and pass in a flag if a copy is required like

	for metric := range unit.src {
		for i, output := range unit.outputs {
			output.AddMetric(metric, i < len(a.Config.Outputs) - 1)
		}
	}

and in the model do

func (r *RunningOutput) AddMetric(m telegraf.Metric, requireCopy bool) {
	metric := m

	ok, err := r.Config.Filter.Select(metric)
	if err != nil {
		r.log.Errorf("filtering failed: %v", err)
	} else if !ok {
		r.metricFiltered(metric)
		return
	}

	if requireCopy {
		metric = m.Copy()
	}

	r.Config.Filter.Modify(metric)
	if len(metric.FieldList()) == 0 {
		r.metricFiltered(metric)
		return
	}
        ...
}

This way we do not need to expose the interna of the output model into the agent.

[LarsStegman]

Yeah, that's also a good solution for me. I will make that change!

[LarsStegman]

@srebhan I am not sure why the memory leak test is failing. I should be making fewer allocations, not more. Do you have any idea?

[srebhan]

@LarsStegman this is unrelated and unfortunately a flaky test... We need to look at it some time but currently things are busy. Ignore the issue for now...

[srebhan]

Awesome! I wonder if we should keep the original AddMetric function signature and always copy the metric and have a second function AddMetricNoCopy which does not copy the metric. This way we save a few ifs and can keep the tests as they were...

[LarsStegman]

That does sound like a better solution to be honest. All the ifs were getting a bit iffy.

[LarsStegman]

Alright, I do like this better!

[srebhan]

Nice. Just get rid of the underscore in the function name and we are good to go.

[srebhan]

Nice! Thanks @LarsStegman!

[srebhan]

@LarsStegman you need this

diff --git a/plugins/inputs/cloud_pubsub_push/cloud_pubsub_push_test.go b/plugins/inputs/cloud_pubsub_push/cloud_pubsub_push_test.go
index 252b843fc..9e8aa07d1 100644
--- a/plugins/inputs/cloud_pubsub_push/cloud_pubsub_push_test.go
+++ b/plugins/inputs/cloud_pubsub_push/cloud_pubsub_push_test.go
@@ -196,6 +196,7 @@ func TestServeHTTP(t *testing.T) {
                        for m := range d {
                                ro.AddMetric(m)
                                ro.Write() //nolint:errcheck // test will fail anyway if the write fails
+                               m.Accept()
                        }
                }(dst)
 

to pass the unit-tests. Those tests are horrible but that's the easiest fix...

[telegraf-tiger]

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

[srebhan]

Looks great! Thanks @LarsStegman!

[DStrand1]

Looks great! Just one minor nitpick: is there any reason this isn't calling r.metricFiltered(metric) like the similar functions?

[LarsStegman]

Yes, that function also calls Drop on the metric, which we should not do if we haven't copied it yet, since we haven't taken ownership of it until then.