This looks good overall! Comments are mostly minor.

Please let me know if you have any questions. Thanks!

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lidizheng)

include/grpcpp/ext/admin_services.h, line 25 at r1 (raw file):

namespace grpc {
namespace experimental {

If we're going to tell people to use this after this release, it should not be in the experimental namespace.

src/cpp/server/admin/admin_services.cc, line 28 at r1 (raw file):

#include <grpcpp/server_builder.h>

#include "src/cpp/server/channelz/channelz_service.h"

Hard-coding these dependencies is okay for now, given that we want to get this in before we cut the next release, but it's not really the way I think this should work in the long run. Please add a TODO here to build a real registration system before we add the next dependency.

src/cpp/server/admin/admin_services.cc, line 38 at r1 (raw file):

namespace {

auto g_channelz_service = absl::make_unique<ChannelzService>();

The style guide prohibits global variables that are not trivially destructible:

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

The end of that section suggests some possible work-arounds.

test/cpp/end2end/admin_services_end2end_test.cc, line 38 at r1 (raw file):

  std::ostringstream s;
  int p = grpc_pick_unused_port_or_die();
  s << "localhost:" << p;

return absl::StrCat("localhost:", p);

test/cpp/end2end/admin_services_end2end_test.cc, line 74 at r1 (raw file):

  }

  std::shared_ptr<Channel> GetChannel() { return channel_; }

This doesn't appear to be used anywhere.

test/cpp/end2end/admin_services_end2end_test.cc, line 86 at r1 (raw file):

};

#ifndef GRPC_NO_XDS

Can just say:

TEST_F(AdminServicesTest, ServicesExported) {
  EXPECT_THAT(GetServiceList(),
              ::testing::UnorderedElementsAre(
#ifndef GRPC_NO_XDS
                  "envoy.service.status.v3.ClientStatusDiscoveryService",
#endif  // !GRPC_NO_XDS
                  "grpc.channelz.v1.Channelz",
                  "grpc.reflection.v1alpha.ServerReflection"));
}

Thanks for the quick review! PTALA.

Reviewable status: 3 of 6 files reviewed, 6 unresolved discussions (waiting on @markdroth)

include/grpcpp/ext/admin_services.h, line 25 at r1 (raw file):

Removed.

Just curious, what's the gRPC Core/C++'s rule for adding new APIs? For gRPC Python API, we usually add an "experimental" note in new APIs; for Java API, I think they add experimental decorators and create issues to track experimental-graduation progress.

src/cpp/server/admin/admin_services.cc, line 28 at r1 (raw file):

Added:

// TODO(lidiz) build a real registration system that can pull in services
// automatically with minimum amount of code.

src/cpp/server/admin/admin_services.cc, line 38 at r1 (raw file):

Applied common pattern No.3, "...use a plain pointer to a dynamically allocated object and never delete it...". ASAN/MSAN seems cool with this.

test/cpp/end2end/admin_services_end2end_test.cc, line 38 at r1 (raw file):

Inlined, since StrCat can shorten this 4 LOC to 1 LOC.

test/cpp/end2end/admin_services_end2end_test.cc, line 74 at r1 (raw file):

Removed, also removed the channel_ member variable.

test/cpp/end2end/admin_services_end2end_test.cc, line 86 at r1 (raw file):

Done. Good idea!

This looks great! Feel free to merge after addressing my one remaining comment.

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lidizheng)

include/grpcpp/ext/admin_services.h, line 25 at r1 (raw file):

The point of marking an API experimental is to let people know that it is still subject to non-backward-compatible changes, so they can't yet count on it to be stable -- any code they write using it may need to change later. But if we're actually going to tell people to use this in Google Cloud documentation, then it needs to be stable before we do that.

The only real prerequisite for adding a non-experimental API is a gRFC. Normally, if we have time and if the API is complex enough, I would prefer to make it experimental and let people use it for a little while before publishing a gRFC and making it non-experimental. But in this case, we need it quickly, and the API is sufficiently simple that I'm not too worried about there being any hidden design flaws that we haven't yet identified.

All of the above should be true across all languages.

src/cpp/server/admin/admin_services.cc, line 39 at r2 (raw file):

static auto* g_channelz_service = new ChannelzService();
static_assert(std::is_trivially_destructible<ChannelzService*>::value, "");

No need for these assertions. Raw pointers (of any type) are always trivially destructible.

Thanks for the review! The "ifndef" usage in test cases was reverted to an earlier version, because MSVC fails if we have "ifndef" statement within macros (e.g., TEST_F and EXPECT_THAT). See log: https://source.cloud.google.com/results/invocations/23b72798-34f1-4209-86e0-571060893144/log

Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @markdroth)

include/grpcpp/ext/admin_services.h, line 25 at r1 (raw file):

Nice to know that we can graduate an API with gRFC. I will follow this usage definition of "experimental" and "stable".

src/cpp/server/admin/admin_services.cc, line 39 at r2 (raw file):

Done.