Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Add the ability to close metric and trace clients #295

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Oct 27, 2021

The OpenTelemetry-collector googlecloud exporter component uses this exporter. If the exporter is Shutdown, it currently only calls StopMetricsExporter and Flush, which just stops the opencensus interval reader. This leaks GRPC client connections, and causes the collector's memory usage to increase.

@google-cla google-cla bot added the cla: yes label Oct 27, 2021
@dashpole dashpole marked this pull request as ready for review October 27, 2021 21:01
@dashpole dashpole requested a review from punya October 28, 2021 01:42
Copy link
Contributor

@punya punya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! Was this related to the memory increase we saw recently, and how did you diagnose it?

@dashpole
Copy link
Contributor Author

dashpole commented Oct 28, 2021

This isn't related to any thing we've seen recently.

I was playing around with the WatchableMapProvider interface in the collector, and noticed it OOMing frequently when I updated the collector config repeatedly. Memory metrics showed an increase in memory usage when the collector's config changed. To debug, I added the pprof extension, and looked at the heap. It showed most of the memory being used by grpc's newHTTP2Client function. Googling told me that it was likely because of grpc client connections not being closed, which made me look here. I found that we weren't closing them, and made this change. I then re-built the collector on top of this commit and verified that the memory leak was gone.

@punya
Copy link
Contributor

punya commented Oct 28, 2021

@dashpole please address the lint failures:

staticcheck ./...
stackdriver.go:443:10: error strings should not be capitalized (ST1005)

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

Successfully merging this pull request may close these issues.

2 participants