Add separate flag for operation tracing (#1119)

Fix a concurrency issue with operation tracing
GoogleCloudDataproc · Feb 22, 2024 · d6fb4d6 · d6fb4d6
1 parent b5cd411
commit d6fb4d6
Show file tree

Hide file tree

Showing 6 changed files with 17 additions and 4 deletions.
diff --git a/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemBase.java b/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemBase.java
@@ -24,9 +24,9 @@
 import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_FILE_CHECKSUM_TYPE;
 import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_GLOB_ALGORITHM;
 import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_LAZY_INITIALIZATION_ENABLE;
+import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_OPERATION_TRACE_LOG_ENABLE;
 import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_OUTPUT_STREAM_SYNC_MIN_INTERVAL_MS;
 import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_OUTPUT_STREAM_TYPE;
-import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_ENABLE;
 import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.GCS_WORKING_DIRECTORY;
 import static com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration.PERMISSIONS_TO_REPORT;
 import static com.google.cloud.hadoop.gcsio.CreateFileOptions.DEFAULT_OVERWRITE;
@@ -498,7 +498,8 @@ public void initialize(URI path, Configuration config) throws IOException {
     // be sufficient (and is required) for the delegation token binding initialization.
     setConf(config);
 
-    this.traceFactory = TraceFactory.get(GCS_TRACE_LOG_ENABLE.get(config, config::getBoolean));
+    this.traceFactory =
+        TraceFactory.get(GCS_OPERATION_TRACE_LOG_ENABLE.get(config, config::getBoolean));
 
     // Initialize the delegation token support, if it is configured
     initializeDelegationTokenSupport(config, path);

diff --git a/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java b/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java
@@ -467,6 +467,11 @@ public class GoogleHadoopFileSystemConfiguration {
   public static final HadoopConfigurationProperty<MetricsSink> GCS_METRICS_SINK =
       new HadoopConfigurationProperty<>("fs.gs.metrics.sink", MetricsSink.NONE);
 
+  /** Configuration key to enable operational level traces */
+  public static final HadoopConfigurationProperty<Boolean> GCS_OPERATION_TRACE_LOG_ENABLE =
+      new HadoopConfigurationProperty<>(
+          "fs.gs.operation.tracelog.enable", GoogleCloudStorageOptions.DEFAULT.isTraceLogEnabled());
+
   /** Configuration key to enable logging of additional trace details. */
   public static final HadoopConfigurationProperty<Boolean> GCS_TRACE_LOG_ENABLE =
       new HadoopConfigurationProperty<>("fs.gs.tracelog.enable", false);
@@ -621,6 +626,7 @@ static GoogleCloudStorageOptions.Builder getGcsOptionsBuilder(Configuration conf
         .setTrafficDirectorEnabled(GCS_GRPC_TRAFFICDIRECTOR_ENABLE.get(config, config::getBoolean))
         .setMetricsSink(GCS_METRICS_SINK.get(config, config::getEnum))
         .setTraceLogEnabled(GCS_TRACE_LOG_ENABLE.get(config, config::getBoolean))
+        .setOperationTraceLogEnabled(GCS_OPERATION_TRACE_LOG_ENABLE.get(config, config::getBoolean))
         .setTraceLogTimeThreshold(GCS_TRACE_LOG_TIME_THRESHOLD_MS.get(config, config::getLong))
         .setTraceLogExcludeProperties(
             ImmutableSet.copyOf(GCS_TRACE_LOG_EXCLUDE_PROPERTIES.getStringCollection(config)));

diff --git a/...src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfigurationTest.java b/...src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfigurationTest.java
@@ -141,6 +141,7 @@ public class GoogleHadoopFileSystemConfigurationTest {
           put("fs.gs.storage.root.url", "https://storage.googleapis.com/");
           put("fs.gs.storage.service.path", "storage/v1/");
           put("fs.gs.tracelog.enable", false);
+          put("fs.gs.operation.tracelog.enable", false);
           put("fs.gs.working.dir", "/");
           put("fs.gs.tracelog.time.filter.threshold.ms", 0L);
           put("fs.gs.tracelog.exclude.properties", ImmutableList.of());

diff --git a/gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemIntegrationTest.java b/gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemIntegrationTest.java
@@ -1492,7 +1492,7 @@ public void verify_storage_statistics_metrics() {
   @Test
   public void testThreadTraceEnabledRename() throws Exception {
     Configuration config = ghfs.getConf();
-    config.set("fs.gs.tracelog.enable", "true");
+    config.set("fs.gs.operation.tracelog.enable", "true");
     ghfs.initialize(ghfs.getUri(), config);
 
     Path testRoot = new Path(sharedBucketName1, "/directory1/");

diff --git a/gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageOptions.java b/gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageOptions.java
@@ -145,6 +145,7 @@ public static Builder builder() {
         .setAuthorizationHandlerProperties(AUTHORIZATION_HANDLER_PROPERTIES_DEFAULT)
         .setMetricsSink(MetricsSink.NONE)
         .setTraceLogEnabled(false)
+        .setOperationTraceLogEnabled(false)
         .setTraceLogTimeThreshold(0)
         .setTraceLogExcludeProperties(ImmutableSet.of())
         .setHnBucketRenameEnabled(SET_HN_BUCKET_CREATE_ENABLED_DEFAULT);
@@ -228,6 +229,8 @@ public static Builder builder() {
 
   public abstract boolean isTraceLogEnabled();
 
+  public abstract boolean isOperationTraceLogEnabled();
+
   public abstract long getTraceLogTimeThreshold();
 
   public abstract ImmutableSet<String> getTraceLogExcludeProperties();
@@ -330,6 +333,8 @@ public abstract Builder setGrpcMessageTimeoutCheckInterval(
 
     public abstract Builder setTraceLogEnabled(Boolean enable);
 
+    public abstract Builder setOperationTraceLogEnabled(Boolean enable);
+
     public abstract Builder setTraceLogTimeThreshold(long threshold);
 
     public abstract Builder setHnBucketRenameEnabled(boolean enabled);

diff --git a/util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java b/util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java
@@ -94,7 +94,7 @@ public String getTrackingId() {
     return this.trackingId;
   }
 
-  ThreadTrace getSubTrace() {
+  synchronized ThreadTrace getSubTrace() {
     long threadId = Thread.currentThread().getId();
 
     subEvents.putIfAbsent(threadId, new ArrayList<>());