admin: add /memory for inspection of heap usage by ambuc · Pull Request #4361 · envoyproxy/envoy

ambuc · 2018-09-06T14:03:38Z

Signed-off-by: James Buckland jbuckland@google.com

Description: Adds a /memory endpoint to the admin panel for fast inspection of Envoy::Memory's heap statistics, without needing to query stats.
Risk Level: Low
Testing: Added a test to admin_test.cc.
Docs Changes: Added a description of the option to admin.rst.
Release Notes: N/A

Signed-off-by: James Buckland <jbuckland@google.com>

htuch · 2018-09-06T14:08:57Z


+Http::Code AdminImpl::handlerMemory(absl::string_view, Http::HeaderMap&, Buffer::Instance& response,
+                                    AdminStream&) {
+  response.add(


Can you use proto format for this? See https://github.com/envoyproxy/envoy/tree/master/api/envoy/admin/v2alpha. This the direction we want to move the API to make it more programmatic and expressable as proto/JSON/etc.

Sure, I will do this instead.

Signed-off-by: James Buckland <jbuckland@google.com>

jmarantz · 2018-09-06T15:31:04Z


+.. http:post:: /memory
+
+  Prints current memory allocation / heap usage, in bytes. Useful in lieu of printing all `/stats` and filtering to get the memory-related statistics.


I don't have a problem with this, but I think it would be not too hard, and pretty useful, to give stats a filtered view -- the filter being applied during map construction, which would have the same benefit but might be more generally . useful, and wouldn't involve adding a new API and proto which would need to live forever.

This is really a matter of taste, though, and I'm fine with this approach if others are.

Agreed, having a filter for a single stat would be useful for stat inspection. But the contents of Memory::Stats aren't really statistics, they're just bundled in there at calltime from MallocExtension. So this seemed like a more direct way to get this bit of information.

Signed-off-by: James Buckland <jbuckland@google.com>

htuch

LGTM, just some nits.

htuch · 2018-09-06T17:10:04Z

+message Memory {
+
+  // The number of bytes allocated by the heap for Envoy.
+  uint64 currently_allocated_bytes = 1;


Do you want to provide references back to the tcmalloc docs for folks who want to understand some of these metrics more deeply?

Sure, I will link them to http://gperftools.github.io/gperftools/tcmalloc.html and try to standardize the field names everywhere.

htuch · 2018-09-06T17:10:39Z

+                                    AdminStream&) {
+  envoy::admin::v2alpha::Memory memory;
+  memory.set_currently_allocated_bytes(Memory::Stats::totalCurrentlyAllocated());
+  memory.set_heap_size_bytes(Memory::Stats::totalCurrentlyReserved());


Should we use the same field names as tcmalloc to minimize any semantic gap?

Those values are already exposed in /stats as:

server.memory_allocated: 70379952 server.memory_heap_size: 74448896

so I'd rather re-use the existing names (either memory_allocated and memory_heap_size or just allocated and heap_size) than introducing new ones.

Why not make a fresh start here? There are no legacy deps and we can aim for alignment with the underlying data source?

I like the idea of allocated and heap_size to match the existing values.

@htuch:

Using different names to represent the same thing only leads to confusion, i.e. is this the same value as the one in /stats and if so why does it have different name?

I don't believe anybody cares that those values come from tcmalloc and not OS or another data source, so the alignment here doesn't seem to provide any value.

Are we forever committed to tcmalloc? Other allocators (e.g. jemalloc) use different names for those stats.

But we're bike-shedding about using currently_allocated_bytes or allocated (since both tcmalloc and /stats use heap_size), so I'm not going to push too much against it.

Arguable by renaming the tcmalloc stats names we are already violating (1). I think (2) is actually not quite right; every time I stare at tcmalloc stats, I need to remind myself specifically what they are measuring, as there is a lot of nuance on each of the different stats it emits (e.g. is this memory allocated, is it allocated but in thread-local cache, is this memory released but not back to the OS, etc.).

I would probably vote to make this a oneof and export based on allocator the specific detailed, allocator defined stats. This is the most useful to the end user, since we don't need to be overly abstract and hide differences.

We can merge this as v2alpha and worry about the bikeshed when we lock down the API. @ambuc can you put in a TODO to consider adding more tcmalloc stats and exploring the naming tradeoffs?

Sounds good -- agreed that these names can be misleading and should be bikeshedded to some degree. I'll merge this as-is and add a TODO for later.

htuch · 2018-09-06T17:10:54Z

+  Http::HeaderMapImpl header_map;
+  Buffer::OwnedImpl response;
+  EXPECT_EQ(Http::Code::OK, getCallback("/memory", header_map, response));
+  std::string output_json = response.toString();


Nit: const std::string

Signed-off-by: James Buckland <jbuckland@google.com>

htuch

Thanks!

Add /memory for via-API inspection of heap usage

648c21d

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc changed the title ~~Add /memory for via-API inspection of heap usage~~ admin: add /memory for inspection of heap usage Sep 6, 2018

htuch reviewed Sep 6, 2018

View reviewed changes

htuch self-assigned this Sep 6, 2018

Return memory.proto instead

e8da810

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc mentioned this pull request Sep 6, 2018

stats: symbolize strings in HeapStatData and ThreadLocalStore #4281

Closed

jmarantz reviewed Sep 6, 2018

View reviewed changes

Fix wrong test assumption

c1b0806

Signed-off-by: James Buckland <jbuckland@google.com>

htuch suggested changes Sep 6, 2018

View reviewed changes

ambuc added 5 commits September 6, 2018 15:48

Rename proto fields to match existing server.memory stats

0f8edcb

Signed-off-by: James Buckland <jbuckland@google.com>

Add link to tcmalloc docs

bcc0bdb

Signed-off-by: James Buckland <jbuckland@google.com>

Add TODO for more tcmalloc/other stats

72225e6

Signed-off-by: James Buckland <jbuckland@google.com>

Merge remote-tracking branch 'upstream/master' into memory-admin

c0018e6

Signed-off-by: James Buckland <jbuckland@google.com>

Merge remote-tracking branch 'upstream/master' into memory-admin

cd708ab

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc mentioned this pull request Sep 7, 2018

admin: add /stats filtering capability #4371

Merged

htuch approved these changes Sep 7, 2018

View reviewed changes

htuch merged commit b5ba7f8 into envoyproxy:master Sep 7, 2018

ambuc deleted the memory-admin branch September 7, 2018 16:32


		.. http:post:: /memory

		Prints current memory allocation / heap usage, in bytes. Useful in lieu of printing all `/stats` and filtering to get the memory-related statistics.

Conversation

ambuc commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants