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

stats: Sink CounterImpl and GaugeImpl into stats_impl.cc; no need to have them in the .h #3701

Merged
merged 2 commits into from
Jun 23, 2018

Conversation

jmarantz
Copy link
Contributor

Description:

My plan is to later templatize the stats impl classes to enable them to be dynamically allocated when not in hot restart, but there's a bit of superfluous exposure that will make that PR more complex than needed, so simplfying that up front. Another step en route to resolving #3585

Risk Level: Low
Testing: //test/...
Docs Changes: N/A
Release Notes: N/A

…em in the .h

Also simplifies the prometheus tests in admin_test.cc for readability,
which seemed natural to me as I removed the references to CounterImpl
and GaugeImpl.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@@ -175,6 +175,70 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) {
}
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this is a simple cut&paste from stats_impl.h.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice cleanup.

@mattklein123 mattklein123 self-assigned this Jun 23, 2018
@mattklein123
Copy link
Member

Please merge master and check test failures (probably master was broken).

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

done; thanks

@mattklein123 mattklein123 merged commit 2eede2f into envoyproxy:master Jun 23, 2018
@jmarantz jmarantz deleted the refactor-admin-stats branch June 23, 2018 20:01
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.

2 participants