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

perf(agent): only copy metric to output when needed #15883

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LarsStegman
Copy link
Contributor

@LarsStegman LarsStegman commented Sep 14, 2024

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
Copy link
Member

srebhan commented Sep 16, 2024

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.

@srebhan srebhan self-assigned this Sep 16, 2024
@LarsStegman
Copy link
Contributor Author

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

@LarsStegman LarsStegman force-pushed the perf/only-copy-metric-when-needed branch from a24bae1 to 032d976 Compare September 17, 2024 11:52
@LarsStegman LarsStegman force-pushed the perf/only-copy-metric-when-needed branch from 784da6d to a61bddf Compare September 17, 2024 13:48
@LarsStegman
Copy link
Contributor Author

@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?

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

Successfully merging this pull request may close these issues.

Many memory allocations with multiple output plugins
2 participants