From 24a39c846709c25093fdf3812de81b1b2b9e40fa Mon Sep 17 00:00:00 2001 From: cloud-profiler-team Date: Wed, 25 Sep 2019 20:47:33 -0700 Subject: [PATCH 1/8] Internal change PiperOrigin-RevId: 271266970 --- third_party/perftools/profiles/proto/builder.cc | 4 ++-- third_party/perftools/profiles/proto/builder.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/third_party/perftools/profiles/proto/builder.cc b/third_party/perftools/profiles/proto/builder.cc index a9f5715cc..81f2272b6 100644 --- a/third_party/perftools/profiles/proto/builder.cc +++ b/third_party/perftools/profiles/proto/builder.cc @@ -89,7 +89,7 @@ uint64 Builder::FunctionId(const char *name, const char *system_name, return index; } -bool Builder::Emit(string *output) { +bool Builder::Emit(std::string *output) { *output = ""; if (!profile_ || !Finalize()) { return false; @@ -97,7 +97,7 @@ bool Builder::Emit(string *output) { return Marshal(*profile_, output); } -bool Builder::Marshal(const Profile &profile, string *output) { +bool Builder::Marshal(const Profile &profile, std::string *output) { *output = ""; StringOutputStream stream(output); GzipOutputStream gzip_stream(&stream); diff --git a/third_party/perftools/profiles/proto/builder.h b/third_party/perftools/profiles/proto/builder.h index 62ae72415..256174cf3 100644 --- a/third_party/perftools/profiles/proto/builder.h +++ b/third_party/perftools/profiles/proto/builder.h @@ -90,12 +90,12 @@ class Builder { // Serializes and compresses the profile into a string, replacing // its contents. It calls Finalize() and returns whether the // encoding was successful. - bool Emit(string *output); + bool Emit(std::string *output); // Serializes and compresses a profile into a string, replacing its // contents. Returns false if there were errors on the serialization // or compression, and the output string will not contain valid data. - static bool Marshal(const Profile &profile, string *output); + static bool Marshal(const Profile &profile, std::string *output); // Serializes and compresses a profile into a file represented by a // file descriptor. Returns false if there were errors on the From 8ee4d5c9a8ed080d8599eb61266252cce28fcb65 Mon Sep 17 00:00:00 2001 From: cloud-profiler-team Date: Thu, 26 Sep 2019 11:14:44 -0700 Subject: [PATCH 2/8] Log & include Java Version in header PiperOrigin-RevId: 271390324 --- src/throttler_api.cc | 37 +++++++++++++++++++++++++++++++++---- src/throttler_api.h | 13 +++++++++++-- src/worker.cc | 2 +- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/throttler_api.cc b/src/throttler_api.cc index c6c311b0c..ec429e53d 100644 --- a/src/throttler_api.cc +++ b/src/throttler_api.cc @@ -248,23 +248,27 @@ bool IsValidServiceName(std::string s) { return true; } -APIThrottler::APIThrottler() - : APIThrottler(DefaultCloudEnv(), DefaultClock(), nullptr) {} +APIThrottler::APIThrottler(JNIEnv* jni) + : APIThrottler(DefaultCloudEnv(), DefaultClock(), nullptr, jni) {} APIThrottler::APIThrottler( CloudEnv* env, Clock* clock, std::unique_ptr - stub) + stub, + JNIEnv* jni) : env_(env), clock_(clock), stub_(std::move(stub)), types_({api::CPU, api::WALL}), + java_version_(JavaVersion(jni)), creation_backoff_envelope_ns_(kBackoffNanos), closed_(false) { grpc_init(); gpr_set_log_function(GRPCLog); + LOG(INFO) << "Java version: " << java_version_; + // Create a random number generator. gen_ = std::default_random_engine(clock_->Now().tv_nsec / 1000); dist_ = std::uniform_int_distribution(0, kRandomRange); @@ -281,6 +285,30 @@ APIThrottler::APIThrottler( } } +std::string APIThrottler::JavaVersion(JNIEnv* jni) { + const std::string kUnknownVersion = "unknown_version"; + + jclass system_class = jni->FindClass("java/lang/System"); + if (system_class == nullptr) { + return kUnknownVersion; + } + jmethodID get_property_method = jni->GetStaticMethodID( + system_class, "getProperty", "(Ljava/lang/String;)Ljava/lang/String;"); + if (get_property_method == nullptr) { + return kUnknownVersion; + } + jstring jstr = reinterpret_cast(jni->CallStaticObjectMethod( + system_class, get_property_method, jni->NewStringUTF("java.version"))); + if (jstr == nullptr) { + return kUnknownVersion; + } + // Copy the returned value and release the memory allocated by JNI. + const char* s = jni->GetStringUTFChars(jstr, nullptr); + std::string ret = string(s); + jni->ReleaseStringUTFChars(jstr, s); + return ret; +} + void APIThrottler::SetProfileTypes(const std::vector& types) { types_ = types; } @@ -397,7 +425,8 @@ void APIThrottler::ResetClientContext() { std::lock_guard lock(ctx_mutex_); ctx_.reset(new grpc::ClientContext()); // NOLINT ctx_->AddMetadata("x-goog-api-client", - "gccl/" + std::string(CLOUD_PROFILER_AGENT_VERSION)); + "gccl/" + std::string(CLOUD_PROFILER_AGENT_VERSION) + + " gl-java/" + java_version_); if (closed_) { ctx_->TryCancel(); diff --git a/src/throttler_api.h b/src/throttler_api.h index 71332ae72..2f9618f1d 100644 --- a/src/throttler_api.h +++ b/src/throttler_api.h @@ -17,10 +17,13 @@ #ifndef CLOUD_PROFILER_AGENT_JAVA_THROTTLER_API_H_ #define CLOUD_PROFILER_AGENT_JAVA_THROTTLER_API_H_ +#include + #include #include #include // NOLINT #include +#include #include #include "src/clock.h" @@ -36,12 +39,13 @@ namespace profiler { // Throttler implementation using the Cloud Profiler API. class APIThrottler : public Throttler { public: - APIThrottler(); + explicit APIThrottler(JNIEnv* jni_env); // Testing-only constructor. APIThrottler(CloudEnv* env, Clock* clock, std::unique_ptr - stub); + stub, + JNIEnv* jni_env); // Set the list of supported profile types. The list is used in the profile // creation call to the server to specify the supported types. @@ -56,6 +60,7 @@ class APIThrottler : public Throttler { void Close() override; private: + FRIEND_TEST(APIThrottlerTest, TestCreatesAndUploadsProfile); // Takes a backoff on profile creation error. The backoff duration // may be specified by the server. Otherwise it will be a randomized // exponentially increasing value, bounded by kMaxBackoffNanos. @@ -64,6 +69,9 @@ class APIThrottler : public Throttler { // Resets the client gRPC context for the next call. void ResetClientContext(); + // Determines and returns Java version. + static std::string JavaVersion(JNIEnv* jni_env); + private: CloudEnv* env_; Clock* clock_; @@ -72,6 +80,7 @@ class APIThrottler : public Throttler { stub_; google::devtools::cloudprofiler::v2::Profile profile_; std::vector types_; + const std::string java_version_; // Profile creation error handling. int64_t creation_backoff_envelope_ns_; diff --git a/src/worker.cc b/src/worker.cc index 457c65116..bf6ba64dd 100644 --- a/src/worker.cc +++ b/src/worker.cc @@ -47,7 +47,7 @@ void Worker::Start(JNIEnv *jni) { // Initialize the throttler here rather in the constructor, since the // constructor is invoked too early, before the heap profiler is initialized. throttler_ = FLAGS_cprof_profile_filename.empty() - ? std::unique_ptr(new APIThrottler()) + ? std::unique_ptr(new APIThrottler(jni)) : std::unique_ptr( new TimedThrottler(FLAGS_cprof_profile_filename)); From ac279814b5bdbc033c102d00e4beee676721400e Mon Sep 17 00:00:00 2001 From: nolanmar Date: Mon, 7 Oct 2019 13:11:51 -0700 Subject: [PATCH 3/8] Add timeouts to CreateProfile and UpdateProfile requests PiperOrigin-RevId: 273359166 --- src/throttler_api.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/throttler_api.cc b/src/throttler_api.cc index ec429e53d..3a56bb489 100644 --- a/src/throttler_api.cc +++ b/src/throttler_api.cc @@ -15,6 +15,7 @@ #include "src/throttler_api.h" #include +#include // NOLINT #include #include "src/clock.h" @@ -335,6 +336,14 @@ bool APIThrottler::WaitNext() { profile_.Clear(); ResetClientContext(); + + // The system clock is used here directly, because clock_->now() returns + // CLOCK_MONOTONIC, not CLOCK_REALTIME time. + // The API server sets a 1 hour server-side timeout. All agents should set + // a corresponding 1 hour timeout for CreateProfile requests. + ctx_->set_deadline(std::chrono::system_clock::now() + + std::chrono::hours{1}); + grpc::Status st = stub_->CreateProfile(ctx_.get(), req, &profile_); if (st.ok()) { LOG(INFO) << "Profile created: " << ProfileType() << " " @@ -387,6 +396,13 @@ bool APIThrottler::Upload(std::string profile) { req.mutable_profile()->set_profile_bytes(std::move(profile)); ResetClientContext(); + + // The system clock is used here directly, because clock_->now() returns + // CLOCK_MONOTONIC, not CLOCK_REALTIME time. + // The API server sets a 20 second server-side timeout. All agents should set + // a corresponding 20 second timeout for UpdateProfile requests. + ctx_->set_deadline(std::chrono::system_clock::now() + + std::chrono::seconds{20}); grpc::Status st = stub_->UpdateProfile(ctx_.get(), req, &profile_); if (!st.ok()) { From 7a046b0969e49f65b95e1e7eb86281bbf5e07169 Mon Sep 17 00:00:00 2001 From: cloud-profiler-team Date: Wed, 9 Oct 2019 09:05:01 -0700 Subject: [PATCH 4/8] Internal change PiperOrigin-RevId: 273757267 --- src/throttler_api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/throttler_api.cc b/src/throttler_api.cc index 3a56bb489..2eb0f05a8 100644 --- a/src/throttler_api.cc +++ b/src/throttler_api.cc @@ -305,7 +305,7 @@ std::string APIThrottler::JavaVersion(JNIEnv* jni) { } // Copy the returned value and release the memory allocated by JNI. const char* s = jni->GetStringUTFChars(jstr, nullptr); - std::string ret = string(s); + std::string ret = std::string(s); jni->ReleaseStringUTFChars(jstr, s); return ret; } From e9e35bf0a1b69a58b890ee5c435063c6a70bb034 Mon Sep 17 00:00:00 2001 From: jcbeyler Date: Mon, 21 Oct 2019 07:51:08 -0700 Subject: [PATCH 5/8] The default behavior is to print out: "heap sampler is not supported, let's disable heap sampler." but it is hard to diagnose why. Provide some low level debug in that case to get more details as to why this has happened. PiperOrigin-RevId: 275841817 --- third_party/javaprofiler/heap_sampler.cc | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/third_party/javaprofiler/heap_sampler.cc b/third_party/javaprofiler/heap_sampler.cc index 1ec292a77..efb499bde 100644 --- a/third_party/javaprofiler/heap_sampler.cc +++ b/third_party/javaprofiler/heap_sampler.cc @@ -208,8 +208,20 @@ bool HeapMonitor::Supported(jvmtiEnv *jvmti) { // If ever this was run with a JDK before JDK11, it would not set this bit as // it was added at the end of the structure. Therefore this is a cheap way to // check for a runtime "are we running with JDK11+". - return caps.can_generate_sampled_object_alloc_events - && caps.can_generate_garbage_collection_events; + if (!caps.can_generate_sampled_object_alloc_events || + !caps.can_generate_garbage_collection_events) { + // Provide more debug information that this will fail: JVMTI_VERSION and + // sizeof is really lower level but helps figure out compilation + // environments. + LOG(WARNING) << "Capabilites not set up: Sampled: " + << caps.can_generate_sampled_object_alloc_events + << "; GC Collection: " + << caps.can_generate_garbage_collection_events + << "; Size of capabilities: " << sizeof(jvmtiCapabilities) + << "; JVMTI_VERSION: " << JVMTI_VERSION; + return false; + } + return true; #else return false; #endif @@ -226,8 +238,8 @@ void HeapMonitor::AddCallback(jvmtiEventCallbacks *callbacks) { bool HeapMonitor::Enable(jvmtiEnv *jvmti, JNIEnv* jni, int sampling_interval) { #ifdef ENABLE_HEAP_SAMPLING if (!Supported(jvmti)) { - LOG(INFO) << "Heap sampling is not supported by the JVM, disabling the " - << " heap sampling monitor"; + LOG(WARNING) << "Heap sampling is not supported by the JVM, disabling the " + << " heap sampling monitor"; return false; } From 508c506b1356717f7bb63e582a385dd78a477d77 Mon Sep 17 00:00:00 2001 From: cloud-profiler-team Date: Sun, 24 Nov 2019 08:40:51 -0800 Subject: [PATCH 6/8] Do not use TEMP_FAILURE_RETRY, which is specific to glibc and non-portable. PiperOrigin-RevId: 282230963 --- third_party/perftools/profiles/proto/builder.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/third_party/perftools/profiles/proto/builder.cc b/third_party/perftools/profiles/proto/builder.cc index 81f2272b6..1e3a29eb9 100644 --- a/third_party/perftools/profiles/proto/builder.cc +++ b/third_party/perftools/profiles/proto/builder.cc @@ -119,8 +119,9 @@ bool Builder::MarshalToFile(const Profile &profile, int fd) { } bool Builder::MarshalToFile(const Profile &profile, const char *filename) { - int fd = - TEMP_FAILURE_RETRY(open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0444)); + int fd; + while ((fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0444)) < 0 && + errno == EINTR) {} if (fd == -1) { PLOG(ERROR) << "Failed to open file " << filename; return false; From 87076cff53ca3874fbc260f00196a45eb81d783d Mon Sep 17 00:00:00 2001 From: nolanmar Date: Mon, 2 Dec 2019 14:30:59 -0800 Subject: [PATCH 7/8] Fix build script PiperOrigin-RevId: 283419171 --- Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 30f686dfd..d70d3f563 100644 --- a/Dockerfile +++ b/Dockerfile @@ -15,7 +15,7 @@ # # Base image # -FROM ubuntu:trusty +FROM ubuntu:xenial # # Dependencies @@ -93,7 +93,7 @@ RUN mkdir /tmp/openssl && cd /tmp/openssl && \ # process of gRPC puts the OpenSSL static object files into the gRPC archive # which causes link errors later when the agent is linked with the static # OpenSSL library itself. -RUN git clone --depth=1 --recursive -b v1.21.0 https://github.com/grpc/grpc.git /tmp/grpc && \ +RUN git clone --depth=1 --recursive -b v1.25.0 https://github.com/grpc/grpc.git /tmp/grpc && \ cd /tmp/grpc && \ # TODO: Remove patch when GKE Istio versions support HTTP 1.0 or From b4165b2b4d86fad499de47f3190544b712c755f3 Mon Sep 17 00:00:00 2001 From: cloud-profiler-team Date: Tue, 3 Dec 2019 14:46:20 -0800 Subject: [PATCH 8/8] Remove HTTP 1.0 hacks for Istio enabled environments. PiperOrigin-RevId: 283626482 --- Dockerfile | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/Dockerfile b/Dockerfile index d70d3f563..47ee03d13 100644 --- a/Dockerfile +++ b/Dockerfile @@ -95,19 +95,6 @@ RUN mkdir /tmp/openssl && cd /tmp/openssl && \ # OpenSSL library itself. RUN git clone --depth=1 --recursive -b v1.25.0 https://github.com/grpc/grpc.git /tmp/grpc && \ cd /tmp/grpc && \ - -# TODO: Remove patch when GKE Istio versions support HTTP 1.0 or -# gRPC http_cli supports HTTP 1.1 -# This sed command is needed until GKE provides Istio 1.1+ exclusively which -# supports HTTP 1.0. - sed -i 's/1\.0/1.1/g' src/core/lib/http/format_request.cc && \ - -# TODO: Remove patch when GKE Istio versions support unambiguous -# FQDNs in rule sets. -# https://github.com/istio/istio/pull/14405 is merged but wait till GKE -# Istio versions includes this PR - sed -i 's/metadata\.google\.internal\./metadata.google.internal/g' src/core/lib/security/credentials/google_default/google_default_credentials.cc && \ - sed -i 's/metadata\.google\.internal\./metadata.google.internal/g' src/core/lib/security/credentials/credentials.h && \ cd third_party/protobuf && \ ./autogen.sh && \ ./configure --with-pic CXXFLAGS="$(pkg-config --cflags protobuf)" LIBS="$(pkg-config --libs protobuf)" LDFLAGS="-Wl,--no-as-needed" && \