[BEAM-4374, BEAM-6189] Delete and remove deprecated Metrics proto #11325
Conversation
This completes the migration off of the Metrics proto to the MonitoringInfo proto.
| // (Optional) Specifies that the bundle has not been completed and the | ||
| // following applications need to be scheduled and executed in the future. | ||
| // A runner that does not yet support residual roots MUST still check that | ||
| // this is empty for correctness. | ||
| repeated DelayedBundleApplication residual_roots = 2; | ||
|
|
||
| // (Required) The list of metrics or other MonitoredState | ||
| // DEPRECATED (Required) The list of metrics or other MonitoredState | ||
| // collected while processing this bundle. | ||
| repeated org.apache.beam.model.pipeline.v1.MonitoringInfo monitoring_infos = 3; |
There was a problem hiding this comment.
Why are these also deprecated now?
Will you be only using the bytes map now? I don't really get how this will work by looking at the ProcessBundle(Progress)Response protos. It seems like you would need the MonitoringInfo at least once to have enough metadata be able to decode the bytes and associate multiple updates together. I don't really understand the protocol that you have in mind here.
There was a problem hiding this comment.
The plan is to go with short ids system as the one and only implementation and may not have enough time to remove these two fields before the 2.21 release.
I want to have the option to un-deprecate them or remove them after the release.
There was a problem hiding this comment.
sgtm, Using ProcessBundleProgressMetadata should be called after the get the IDs, makes sense.
| updateMetrics(monitoringInfosList); | ||
| updateMetricsDeprecated(metrics); |
There was a problem hiding this comment.
Are you sure this won't break existing integrations? I believe only the Python and Java SDK have moved everything to MonitoringInfos. The Go SDK has not yet
There was a problem hiding this comment.
Go SDK has, @lostluck and I worked on the changes. See:
| userMetric = metrics.getPtransformsOrThrow(stepName).getUser(0); | ||
| assertThat(userMetric.getMetricName(), equalTo(metricName)); | ||
| assertThat(userMetric.getCounterData().getValue(), equalTo(finalCounterValue)); | ||
| } |
There was a problem hiding this comment.
You way want to add this back, if you truly cannot yet delete the deprecated metrics
There was a problem hiding this comment.
Which deprecated metrics are you referring to?
There was a problem hiding this comment.
The legacy metrics proto deleted in this PR. You may want to check with to make sure no other runner is consuming this.
|
CC: @HuangLED |
|
LGTM for Go |
|
LGTM overall. |
|
Run Python PreCommit |
| // (Optional) Specifies that the bundle has not been completed and the | ||
| // following applications need to be scheduled and executed in the future. | ||
| // A runner that does not yet support residual roots MUST still check that | ||
| // this is empty for correctness. | ||
| repeated DelayedBundleApplication residual_roots = 2; | ||
|
|
||
| // (Required) The list of metrics or other MonitoredState | ||
| // DEPRECATED (Required) The list of metrics or other MonitoredState | ||
| // collected while processing this bundle. | ||
| repeated org.apache.beam.model.pipeline.v1.MonitoringInfo monitoring_infos = 3; |
There was a problem hiding this comment.
sgtm, Using ProcessBundleProgressMetadata should be called after the get the IDs, makes sense.
| updateMetrics(monitoringInfosList); | ||
| updateMetricsDeprecated(metrics); |
| userMetric = metrics.getPtransformsOrThrow(stepName).getUser(0); | ||
| assertThat(userMetric.getMetricName(), equalTo(metricName)); | ||
| assertThat(userMetric.getCounterData().getValue(), equalTo(finalCounterValue)); | ||
| } |
There was a problem hiding this comment.
The legacy metrics proto deleted in this PR. You may want to check with to make sure no other runner is consuming this.
|
Run Java PreCommit |
This completes the migration off of the Metrics proto to the MonitoringInfo proto.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.