Add support for operation tracing #1075

arunkumarchacko · 2023-11-17T04:57:31Z

No description provided.

arunkumarchacko · 2023-11-17T04:57:44Z

/gcbrun

arunkumarchacko · 2023-11-17T05:12:10Z

/gcbrun

arunkumarchacko · 2023-11-17T12:04:38Z

/gcbrun

arunkumarchacko · 2023-11-17T12:14:08Z

/gcbrun

arunkumarchacko · 2023-11-29T05:05:51Z

/gcbrun

singhravidutt · 2023-12-01T08:10:26Z

util/src/main/java/com/google/cloud/hadoop/util/ITraceFactory.java

@@ -0,0 +1,21 @@
+/*
+ * Copyright 2013 Google Inc.


util/src/main/java/com/google/cloud/hadoop/util/TraceFactory.java

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystem.java

gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemIntegrationTest.java

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/BatchHelper.java

singhravidutt · 2023-12-05T08:42:48Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/BatchHelper.java

      throws IOException {
-    try {
+    String traceName = this.traceContext + "(size=1)";


what is the significance of (size=1) is it the batchSize?

If the control reaches here, the batchsize will be 1. Hence adding that context.

Can we follow the same format then as in L-245 i.e. batchSize= instead of size=1?

Or if it is intentionally different?

singhravidutt · 2023-12-05T08:59:02Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageImpl.java

+              storageOptions.getMaxRequestsPerBatch(),
+              resourceIds.size(),
+              storageOptions.getBatchThreads(),
+              "batchGetItemInfo");


shouldn't this be traceContext which we prepared above ?

singhravidutt · 2023-12-05T09:02:47Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/BatchHelper.java

@@ -227,7 +242,10 @@ private void flushPendingRequests() throws IOException {
    responseFutures.add(
        requestsExecutor.submit(
            () -> {
-              batch.execute();
+              String tc = String.format("%s(batchSize=%s)", this.traceContext, batch.size());


nit:shouldn't this be traceName instead of tc?

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

singhravidutt · 2023-12-07T11:31:21Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+
+    ThreadTraceEvent endEvent() {
+      ThreadTraceEvent result = new ThreadTraceEvent(EventType.END, name);
+      result.setTimeTaken(result.timeStamp - timeStamp);


is it even capturing anything? result will be created in L163 and time taken is getting updated just immediately which will be tending to 0.

It is finding the difference of result with the timestamp of "this" event, which might have happened in past.

mind using this for timestamp?

why we need a separate event(i.e. END) for it? why can't we maintain a state of an operation to be either Started Completed, Merged?

Not sure if I understood the comment. Start indicate when the operation started and End indicates when the operation has ended. Merged is a special kind of event which merges two consecutive start-end event pairs to one.

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

singhravidutt · 2023-12-08T10:17:31Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+
+    ThreadTraceEvent endEvent() {
+      ThreadTraceEvent result = new ThreadTraceEvent(EventType.END, name);
+      result.setTimeTaken(result.timeStamp - timeStamp);


mind using this for timestamp?

singhravidutt · 2023-12-08T10:25:14Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+      }
+    }
+
+    public void markEnd() {


via this function we are updating the type of the event which gives a feeling that EventType defined above is not actually the type but the state of an event. Which is very confusing.

also function says markEnd and inside we are updating the type to MERGED which is also adding to the confusion.

What are your thoughts?

We are merging the events to reduce the number of events. Added a comment.

singhravidutt · 2023-12-08T10:28:58Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+
+    ThreadTraceEvent endEvent() {
+      ThreadTraceEvent result = new ThreadTraceEvent(EventType.END, name);
+      result.setTimeTaken(result.timeStamp - timeStamp);


why we need a separate event(i.e. END) for it? why can't we maintain a state of an operation to be either Started Completed, Merged?

util/src/main/java/com/google/cloud/hadoop/util/TraceFactory.java

singhravidutt · 2023-12-08T10:38:51Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/BatchHelper.java

      throws IOException {
-    try {
+    String traceName = this.traceContext + "(size=1)";


Can we follow the same format then as in L-245 i.e. batchSize= instead of size=1?

Or if it is intentionally different?

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

singhravidutt · 2023-12-11T10:24:12Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+      }
+    }
+
+    public void markEnd() {


shouldn't we verify that this is of type START before updating the type to MERGED?

Shouldn't we capture the endtime as well just for completeness purposes and maybe pass the endTime as an argument to this function.

This will always be start since it is created in the constructor.

If we use endtime, we still need to subtract to find the time taken. We are more interested in the time taken by the operation. endTime can be derived from startTime and timeTaken. There is no value in duplicating the same information.

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

singhravidutt · 2023-12-11T10:48:51Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+
+    int eventsSize = events.size();
+    if (eventsSize != 0) {
+      ThreadTraceEvent lastEvent = events.get(eventsSize - 1);


One doubt: why we need to match it with the most recent event(or last) in array? We have reference to the startEvent and now an End is been invoked on it. Why not just go ahead and update the type of START event to be of type MERGED instead off creating an END type event?

For eg. op1.start -> op2.start -> op2.end ->op1.end It is good to know the tree structure of the operations.

Can we have these intensions captured somewhere. It's will be hard to follow later on.

singhravidutt · 2023-12-11T16:12:53Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+    }
+
+    ThreadTraceEvent endEvent() {
+      ThreadTraceEvent result = new ThreadTraceEvent(EventType.END, name);


nit: this.name

We usually add "this" in the construction.

singhravidutt · 2023-12-11T16:14:10Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+      return new ThreadTraceEvent(EventType.START, name);
+    }
+
+    private ThreadTraceEvent(EventType type, String name) {


would name suffice or do we need a unique id too?

The name already has some uniqueness. Basically it will be the context followed by current milliseconds.

singhravidutt · 2023-12-11T16:14:59Z

util/src/main/java/com/google/cloud/hadoop/util/ThreadTrace.java

+
+    int eventsSize = events.size();
+    if (eventsSize != 0) {
+      ThreadTraceEvent lastEvent = events.get(eventsSize - 1);


Can we have these intensions captured somewhere. It's will be hard to follow later on.

arunkumarchacko · 2023-12-11T16:33:02Z

/gcbrun

arunkumarchacko · 2023-12-12T13:52:25Z

/gcbrun

arunkumarchacko · 2023-12-12T14:06:44Z

/gcbrun

arunkumarchacko · 2023-12-12T14:55:57Z

/gcbrun

arunkumarchacko · 2023-12-13T06:19:00Z

/gcbrun

arunkumarchacko · 2023-12-13T07:15:30Z

/gcbrun

arunkumarchacko · 2023-12-13T14:31:04Z

/gcbrun

arunkumarchacko · 2023-12-14T01:23:40Z

/gcbrun

Add support for operation tracing

58c1dde

Fix code format violations

24b4239

arunkumarchacko requested a review from singhravidutt November 17, 2023 05:40

Enable operation tracing based on config

d1cce47

Fix build

fba8e0a

singhravidutt reviewed Dec 5, 2023

View reviewed changes

Take CR comments

1433c88

singhravidutt reviewed Dec 7, 2023

View reviewed changes

singhravidutt reviewed Dec 8, 2023

View reviewed changes

Take code review comments

d8ea022

singhravidutt reviewed Dec 11, 2023

View reviewed changes

singhravidutt approved these changes Dec 11, 2023

View reviewed changes

Merge branch 'master' into tracing_master

1e97b33

arunkumarchacko requested a review from singhravidutt December 14, 2023 02:45

singhravidutt approved these changes Dec 14, 2023

View reviewed changes

Merge branch 'master' into tracing_master

d3300d2

arunkumarchacko merged commit 831bc38 into GoogleCloudDataproc:master Dec 14, 2023
1 of 2 checks passed

arunkumarchacko deleted the tracing_master branch December 14, 2023 09:22

arunkumarchacko added a commit to arunkumarchacko/hadoop-connectors that referenced this pull request Dec 19, 2023

Add support for operation tracing (GoogleCloudDataproc#1075)

6bc356b

arunkumarchacko added a commit that referenced this pull request Jan 1, 2024

Add support for operation tracing (#1075) (#1088)

68cd68b

arunkumarchacko added a commit to arunkumarchacko/hadoop-connectors that referenced this pull request Jan 3, 2024

Merge PR GoogleCloudDataproc#1075

69b6d23

arunkumarchacko mentioned this pull request Jan 3, 2024

Add support for operation tracing #1098

Merged

arunkumarchacko added a commit that referenced this pull request Jan 4, 2024

Merge PR #1075 (#1098)

a9dd738

Add support for operation tracing #1075

Add support for operation tracing #1075

Conversation

arunkumarchacko commented Nov 17, 2023

arunkumarchacko commented Nov 17, 2023

arunkumarchacko commented Nov 17, 2023

arunkumarchacko commented Nov 17, 2023

arunkumarchacko commented Nov 17, 2023

arunkumarchacko commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkumarchacko commented Dec 11, 2023

arunkumarchacko commented Dec 12, 2023

arunkumarchacko commented Dec 12, 2023

arunkumarchacko commented Dec 12, 2023

arunkumarchacko commented Dec 13, 2023

arunkumarchacko commented Dec 13, 2023

arunkumarchacko commented Dec 13, 2023

arunkumarchacko commented Dec 14, 2023