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

Creating several NewRelic structs uses too much memory #576

Open
Tarliton opened this issue Dec 17, 2020 · 1 comment
Open

Creating several NewRelic structs uses too much memory #576

Tarliton opened this issue Dec 17, 2020 · 1 comment
Labels
bug Something isn't working client-go pinned Pin an issue to prevent auto-closing by a bot.

Comments

@Tarliton
Copy link
Contributor

Description

Hello.
If we call newrelic.New several times we end up using too much memory. Even when we do not keep all those structs in memory at the same time.

That is because everytime we create a new NewRelic struct a new Config struct is created.
That config is then sent to accounts.New here.
In accounts.New we end up calling config.GetLogger.
In config.GetLogger we create a new logger if this config has not already created one. This happens only inside newrelic.New function, not when we create a different NewRelic struct.
When we create a new StructuredLogger we end up calling SetDefaultFields. That function calls logrus's log.AddHook.
At last, here is the problem: logrus log is a global/package variable and we end up adding several hooks to it.

Go Version

go version go1.15.6 linux/amd64

Current behavior

Creating and destroying several NewRelic structs with newrelic.New function uses too much memory.

Expected behavior

I would expect to be able to create and destroy several NewRelic structs with newrelic.New without using too much memory.

Steps To Reproduce

Steps to reproduce the behavior:

  1. Create a benchmark calling newrelic.New function
// +build unit

package newrelic

var nr *NewRelic
var err error

func BenchmarkNew(b *testing.B) {
	cfgOpt := ConfigPersonalAPIKey(testAPIkey)

	for n := 0; n < b.N; n++ {
		nr, err = New(cfgOpt)
	}

}
  1. Run it with:
go test -tags unit -memprofile=mem.out -v ./newrelic/... -bench=BenchmarkNew -benchmem -benchtime=20s
  1. Watch your memory usage increase
  2. Check results with:
go tool pprof -http :8081 mem.out
  1. VIEW ->Top
    1nr

Additional Context

I'm aware that we should reuse NewRelic struct. I found this issue by accident.
Should we let StructuredLogger.SetDefaultFields aware of this global behavior of logrus log with a sync.Once variable?
I was able to decrease memory usage with one, but I do not know if this is the right solution.
2nr

Thank you

@ctrombley ctrombley added bug Something isn't working pinned Pin an issue to prevent auto-closing by a bot. labels Dec 28, 2020
@ctrombley
Copy link
Contributor

Thanks for reporting this issue and for all of the detailed context! We can probably make better reuse of the config when instantiating a new NewRelic container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client-go pinned Pin an issue to prevent auto-closing by a bot.
Projects
None yet
Development

No branches or pull requests

3 participants