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

Implement configurable caching #46

Merged
merged 2 commits into from Sep 29, 2020
Merged

Implement configurable caching #46

merged 2 commits into from Sep 29, 2020

Conversation

Phil1602
Copy link
Contributor

Hi,

thanks for this exporter.

We have many Promethei, which are scraping in parallel with a small scrape interval. Unfortunately we discovered load issues because of the DellHW Exporter respectively the expensive underlying omreport calls (all collectors were enabled) on our machines.

We decided to implement a simple in memory cache, which can be enabled using the configuration flags, to face this problem. The caching is optional and can be enabled using the configuration parameters.

Implementation details:
I decided to implement an additional adapter channel, which will be used to retrieve the collected metrics and put them into an array if caching is enabled. A mutex is used to prevent concurrent collections and concurrent write operations to the cache.

Since the metrics are written into a "local" channel instead of the channel passed by the Prometheus library directly, we need a second waitgroup. The first waitgroup ensures that all collectors have finished. The second waitgroup ensures that all metrics are written to the outgoing channel before the method returns. This is needed because the Prometheus library will close the channel once the method returns.

There was already a field for the last collection timestamp, which was not actually used before. The initial timestamp will now be Unix 0 instead of the current time (start time of the exporter), because otherwise the actual collection would not be executed within the first cache period if caching is enabled. Also the receiver had to be changed to Pointer instead of Value, because the updated timestamp was only updated for the copy in case of the value receiver.

The new configuration parameters have been added to the configuration documentation. Shall I add some more documentation to further describe the caching behavior for the end user? What about the debug logs, are they ok or shall I remove them?

Thanks!
Philipp

Copy link
Owner

@galexrt galexrt 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 the PR!

Adding the cache implementation details as a new page to the docs sounds good.
I might "move" it around in the doc structure later on, but don't worry about that.

I only have one small comment regarding the code.

cmd/dellhw_exporter/dellhw_exporter.go Outdated Show resolved Hide resolved
@Phil1602
Copy link
Contributor Author

I will add the documentation page tomorrow.

Copy link
Owner

@galexrt galexrt left a comment

Choose a reason for hiding this comment

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

LGTM, just some nit picks regarding the docs.
I'll merge after those have been fixed.

README.md Outdated Show resolved Hide resolved
docs/caching.md Outdated Show resolved Hide resolved
docs/caching.md Outdated Show resolved Hide resolved
@Phil1602
Copy link
Contributor Author

Sorry I didn't had time yesterday.
The documention has been added.

@Phil1602
Copy link
Contributor Author

Applied your suggestions and squashed the documentation related commits together.

@galexrt
Copy link
Owner

galexrt commented Sep 29, 2020

@Phil1602 Thanks for the PR and quick updates!

I'll push a new release in a few hours after the merge.

@galexrt galexrt merged commit 426b518 into galexrt:master Sep 29, 2020
@galexrt
Copy link
Owner

galexrt commented Sep 29, 2020

@Phil1602 v1.7.0 has been released.

galexrt pushed a commit that referenced this pull request Oct 3, 2020
build: updated travis ci github token
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.

None yet

2 participants