You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/10/21 19:39:35 UTC

[GitHub] [beam] ajamato opened a new pull request #13163: [DO NOT SUBMIT] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

ajamato opened a new pull request #13163:
URL: https://github.com/apache/beam/pull/13163


   [DO NOT SUBMIT] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   Post-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
   --- | --- | --- | --- | --- | --- | ---
   Go | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
   Java | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/i
 con)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[![Build Status](htt
 ps://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
   Python | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam
 .apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
   XLang | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   --- |Java | Python | Go | Website | Whitespace | Typescript
   --- | --- | --- | --- | --- | --- | ---
   Non-portable | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/be
 am_PreCommit_Go_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
   Portable | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
   
   See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
   
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ajamato commented on pull request #13163: [DO NOT SUBMIT] [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
ajamato commented on pull request #13163:
URL: https://github.com/apache/beam/pull/13163#issuecomment-713909876


   retest this please


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ajamato commented on pull request #13163: [DO NOT SUBMIT] [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
ajamato commented on pull request #13163:
URL: https://github.com/apache/beam/pull/13163#issuecomment-713976788


   Run Python_PVR_Flink PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ajamato commented on a change in pull request #13163: [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13163:
URL: https://github.com/apache/beam/pull/13163#discussion_r510506861



##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -306,18 +351,18 @@ message ProcessBundleProgressRequest {
   string instruction_id = 1;
 }
 
-// A request to provide full MonitoringInfo for a given id.
+// A request to provide full MonitoringInfo for a given bundle.
 //
 // An SDK can report metrics using an identifier that only contains the
 // associated payload. A runner who wants to receive the full metrics
 // information can request all the monitoring metadata via a
 // MonitoringInfosMetadataRequest providing a list of ids as necessary.
 //
-// The MonitoringInfo ids are scoped to the associated control connection. For
-// example, an SDK may reuse the ids across multiple bundles.
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
 message MonitoringInfosMetadataRequest {
-  // A list of ids for which MonitoringInfo are requested. All but the payload
-  // field will be populated.
+  // A list of ids for which the full MonitoringInfo is requested for.
   repeated string monitoring_info_id = 1;
 }
 

Review comment:
       I believe I have applied all of your suggestions now. I did some through this webpage and some locally.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ajamato commented on pull request #13163: [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
ajamato commented on pull request #13163:
URL: https://github.com/apache/beam/pull/13163#issuecomment-714818638


   Run Python_PVR_Flink PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ajamato commented on pull request #13163: [DO NOT SUBMIT] [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
ajamato commented on pull request #13163:
URL: https://github.com/apache/beam/pull/13163#issuecomment-713976181






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ajamato commented on pull request #13163: [DO NOT SUBMIT] [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
ajamato commented on pull request #13163:
URL: https://github.com/apache/beam/pull/13163#issuecomment-714064847


   Run Python PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ajamato commented on pull request #13163: [DO NOT SUBMIT] [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
ajamato commented on pull request #13163:
URL: https://github.com/apache/beam/pull/13163#issuecomment-714637332


   R: @lukecwik
   
   Would you PTAL. I believe the current failures are being addressed here:
   https://issues.apache.org/jira/browse/BEAM-11101?jql=text%20~%20%22SnippetsTest%22
   
   https://issues.apache.org/jira/browse/BEAM-10921?jql=text%20~%20%22apache_beam%5C%5Crunners%5C%5Cinteractive%5C%5Crecording_manager_test.py%22


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] lukecwik commented on a change in pull request #13163: [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #13163:
URL: https://github.com/apache/beam/pull/13163#discussion_r510480672



##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
     ProcessBundleSplitResponse process_bundle_split = 1003;
     FinalizeBundleResponse finalize_bundle = 1004;
     MonitoringInfosMetadataResponse monitoring_infos = 1005;
+    HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+    HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
 
     // DEPRECATED
     RegisterResponse register = 1000;
   }
 }
 
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.

Review comment:
       ```suggestion
   // The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
   // as the MonitoringInfo could be reconstructed fully by overwriting its
   // payload field with the bytes specified here.
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -108,6 +108,8 @@ message InstructionRequest {
     ProcessBundleSplitRequest process_bundle_split = 1003;
     FinalizeBundleRequest finalize_bundle = 1004;
     MonitoringInfosMetadataRequest monitoring_infos = 1005;
+    HarnessMonitoringInfosRequest harness_monitoring_infos = 1006;
+    HarnessMonitoringInfosMetadataRequest harness_monitoring_infos_metadata = 1007;

Review comment:
       I thought that this would be covered by MonitoringInfosMetadataRequest
   ```suggestion
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
     ProcessBundleSplitResponse process_bundle_split = 1003;
     FinalizeBundleResponse finalize_bundle = 1004;
     MonitoringInfosMetadataResponse monitoring_infos = 1005;
+    HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+    HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
 
     // DEPRECATED
     RegisterResponse register = 1000;
   }
 }
 
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
+message HarnessMonitoringInfosRequest {
+}
+
+message HarnessMonitoringInfosResponse {
+  // An identifier to MonitoringInfo.payload mapping containing
+  // Metrics associated with the sdk harness process, not a specific bundle.
+  //
+  // An SDK can report metrics using an identifier that only contains the
+  // associated payload. A runner who wants to receive the full metrics
+  // information can request all the monitoring metadata via a
+  // MonitoringInfosMetadataRequest providing a list of ids as necessary.
+  //
+  // The SDK is allowed to reuse the identifiers across multiple bundles as long

Review comment:
       ```suggestion
     // The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -306,18 +351,18 @@ message ProcessBundleProgressRequest {
   string instruction_id = 1;
 }
 
-// A request to provide full MonitoringInfo for a given id.
+// A request to provide full MonitoringInfo for a given bundle.

Review comment:
       ```suggestion
   // A request to provide full MonitoringInfo for a set of provided ids.
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
     ProcessBundleSplitResponse process_bundle_split = 1003;
     FinalizeBundleResponse finalize_bundle = 1004;
     MonitoringInfosMetadataResponse monitoring_infos = 1005;
+    HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+    HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;

Review comment:
       ```suggestion
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -349,11 +394,11 @@ message ProcessBundleProgressResponse {
 // information can request all the monitoring metadata via a
 // MonitoringInfosMetadataRequest providing a list of ids as necessary.
 //
-// The MonitoringInfo ids are scoped to the associated control connection. For
-// example an SDK may reuse the ids across multiple bundles.
+// The SDK is allowed to reuse the identifiers across multiple bundles as long

Review comment:
       ```suggestion
   // The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
     ProcessBundleSplitResponse process_bundle_split = 1003;
     FinalizeBundleResponse finalize_bundle = 1004;
     MonitoringInfosMetadataResponse monitoring_infos = 1005;
+    HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+    HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
 
     // DEPRECATED
     RegisterResponse register = 1000;
   }
 }
 
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
+message HarnessMonitoringInfosRequest {
+}
+
+message HarnessMonitoringInfosResponse {
+  // An identifier to MonitoringInfo.payload mapping containing
+  // Metrics associated with the sdk harness process, not a specific bundle.

Review comment:
       ```suggestion
     // Metrics associated with the SDK harness, not a specific bundle.
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -306,18 +351,18 @@ message ProcessBundleProgressRequest {
   string instruction_id = 1;
 }
 
-// A request to provide full MonitoringInfo for a given id.
+// A request to provide full MonitoringInfo for a given bundle.
 //
 // An SDK can report metrics using an identifier that only contains the
 // associated payload. A runner who wants to receive the full metrics
 // information can request all the monitoring metadata via a
 // MonitoringInfosMetadataRequest providing a list of ids as necessary.
 //
-// The MonitoringInfo ids are scoped to the associated control connection. For
-// example, an SDK may reuse the ids across multiple bundles.
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
 message MonitoringInfosMetadataRequest {
-  // A list of ids for which MonitoringInfo are requested. All but the payload
-  // field will be populated.
+  // A list of ids for which the full MonitoringInfo is requested for.
   repeated string monitoring_info_id = 1;
 }
 

Review comment:
       Can you copy the update to say `for the lifetime of the associated control connection` in ProcessBundleProgressResponse and ProcessBundleResponse

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -306,18 +351,18 @@ message ProcessBundleProgressRequest {
   string instruction_id = 1;
 }
 
-// A request to provide full MonitoringInfo for a given id.
+// A request to provide full MonitoringInfo for a given bundle.
 //
 // An SDK can report metrics using an identifier that only contains the
 // associated payload. A runner who wants to receive the full metrics
 // information can request all the monitoring metadata via a
 // MonitoringInfosMetadataRequest providing a list of ids as necessary.
 //
-// The MonitoringInfo ids are scoped to the associated control connection. For
-// example, an SDK may reuse the ids across multiple bundles.
+// The SDK is allowed to reuse the identifiers across multiple bundles as long

Review comment:
       ```suggestion
   // The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
   ```

##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
     ProcessBundleSplitResponse process_bundle_split = 1003;
     FinalizeBundleResponse finalize_bundle = 1004;
     MonitoringInfosMetadataResponse monitoring_infos = 1005;
+    HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+    HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
 
     // DEPRECATED
     RegisterResponse register = 1000;
   }
 }
 
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
+message HarnessMonitoringInfosRequest {
+}
+
+message HarnessMonitoringInfosResponse {
+  // An identifier to MonitoringInfo.payload mapping containing
+  // Metrics associated with the sdk harness process, not a specific bundle.
+  //
+  // An SDK can report metrics using an identifier that only contains the
+  // associated payload. A runner who wants to receive the full metrics
+  // information can request all the monitoring metadata via a
+  // MonitoringInfosMetadataRequest providing a list of ids as necessary.
+  //
+  // The SDK is allowed to reuse the identifiers across multiple bundles as long
+  // as the MonitoringInfo could be reconstructed fully by overwriting its
+  // payload field with the bytes specified here.
+  map<string, bytes> monitoring_data = 1;
+}
+
+message HarnessMonitoringInfosMetadataRequest {
+  // A list of ids for which the full MonitoringInfo is requested for.
+  repeated string monitoring_info_id = 1;
+}
+
+message HarnessMonitoringInfosMetadataResponse {
+  // A mapping from an identifier to the full metrics information.
+  // The “bytes payload” field is not populated on the MonitoringInfo
+  map<string, org.apache.beam.model.pipeline.v1.MonitoringInfo>
+      monitoring_infos = 1;
+}
+

Review comment:
       ```suggestion
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] lukecwik merged pull request #13163: [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics

Posted by GitBox <gi...@apache.org>.
lukecwik merged pull request #13163:
URL: https://github.com/apache/beam/pull/13163


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org