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 2022/06/02 21:50:04 UTC
[GitHub] [beam] riteshghorse opened a new pull request, #17821: Remove monitoring infos
riteshghorse opened a new pull request, #17821:
URL: https://github.com/apache/beam/pull/17821
Currently we export metrics with both MonitoringInfos and MonitoringData. Several runs were performed that confirms that the dataflow gets the metrics both ways. So we would like to get rid of MonitoringInfos in ProcessBundleResponse. See https://github.com/apache/beam/blob/12be69dcd14df43e314a4c3abb086a7066c2a6a0/sdks/go/pkg/beam/model/fnexecution_v1/beam_fn_api.pb.go#L1364 for more details
------------------------
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`).
- [ ] Add a link to the appropriate issue in your description, 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).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17821: Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1145377667
Can one of the admins verify this patch?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #17821: Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1145380447
# [Codecov](https://codecov.io/gh/apache/beam/pull/17821?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17821](https://codecov.io/gh/apache/beam/pull/17821?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ff88da1) into [master](https://codecov.io/gh/apache/beam/commit/56755f5819e0e4bd36104ab66535d005609e5503?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (56755f5) will **increase** coverage by `0.05%`.
> The diff coverage is `12.50%`.
```diff
@@ Coverage Diff @@
## master #17821 +/- ##
==========================================
+ Coverage 74.09% 74.14% +0.05%
==========================================
Files 697 697
Lines 91986 91921 -65
==========================================
Hits 68154 68154
+ Misses 22583 22518 -65
Partials 1249 1249
```
| Flag | Coverage Δ | |
|---|---|---|
| go | `50.95% <12.50%> (+0.12%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/17821?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [sdks/go/pkg/beam/core/runtime/harness/harness.go](https://codecov.io/gh/apache/beam/pull/17821/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvaGFybmVzcy9oYXJuZXNzLmdv) | `10.50% <0.00%> (+0.04%)` | :arrow_up: |
| [...dks/go/pkg/beam/core/runtime/harness/monitoring.go](https://codecov.io/gh/apache/beam/pull/17821/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvaGFybmVzcy9tb25pdG9yaW5nLmdv) | `41.57% <0.00%> (+17.23%)` | :arrow_up: |
| [sdks/go/pkg/beam/core/runtime/graphx/translate.go](https://codecov.io/gh/apache/beam/pull/17821/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZ3JhcGh4L3RyYW5zbGF0ZS5nbw==) | `43.13% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17821?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17821?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [56755f5...ff88da1](https://codecov.io/gh/apache/beam/pull/17821?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #17821: [BEAM-14557] Read and Seek Runner Capabilities in Go SDK
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1154628318
https://ci-beam.apache.org/job/beam_PostCommit_Go_PR/438/ passed! Merging this.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on a diff in pull request #17821: [BEAM-14557] Read and Seek Runner Capabilities in Go SDK
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on code in PR #17821:
URL: https://github.com/apache/beam/pull/17821#discussion_r894880906
##########
sdks/go/pkg/beam/core/runtime/harness/harness.go:
##########
@@ -371,6 +390,10 @@ func (c *control) handleInstruction(ctx context.Context, req *fnpb.InstructionRe
c.cache.CompleteBundle(tokens...)
mons, pylds := monitoring(plan, store)
+ if c.runnerCapabilities[URNMonitoringInfoShortID] {
+ mons = []*pipeline_v1.MonitoringInfo{}
+ }
Review Comment:
yeah, that is taking up memory as well as space. Fixed.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #17821: [BEAM-14557] Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1151700493
As discussed, we can't remove the monitoring infos due to other runners, but I have found the correct URNs we need to look out for and probably provide as a capability:
"beam:protocol:monitoring_info_short_ids:v1"
https://source.corp.google.com/piper///depot/google3/third_party/apache_beam/model/pipeline/org/apache/beam/model/pipeline/v1/beam_runner_api.proto;l=1639?q=StandardRunnerProtocols&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #17821: [BEAM-14557] Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1146022317
Okay, it looks like the universal runner extracts metrics from MonitoringInfos for producing the pipeline result. Need to change that as well.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #17821: [BEAM-14557] Read and Seek Runner Capabilities in Go SDK
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1154560591
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17821: Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1145377668
Can one of the admins verify this patch?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17821: Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1145377678
Can one of the admins verify this patch?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17821: Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1145378745
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on a diff in pull request #17821: [BEAM-14557] Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on code in PR #17821:
URL: https://github.com/apache/beam/pull/17821#discussion_r894840369
##########
sdks/go/pkg/beam/core/runtime/graphx/translate.go:
##########
@@ -86,7 +86,7 @@ const (
func goCapabilities() []string {
capabilities := []string{
- URNLegacyProgressReporting,
+ URNProgressReporting,
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on a diff in pull request #17821: [BEAM-14557] Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #17821:
URL: https://github.com/apache/beam/pull/17821#discussion_r894847247
##########
sdks/go/pkg/beam/core/runtime/harness/harness.go:
##########
@@ -371,6 +390,10 @@ func (c *control) handleInstruction(ctx context.Context, req *fnpb.InstructionRe
c.cache.CompleteBundle(tokens...)
mons, pylds := monitoring(plan, store)
+ if c.runnerCapabilities[URNMonitoringInfoShortID] {
+ mons = []*pipeline_v1.MonitoringInfo{}
+ }
Review Comment:
So this only accomplishes half the goal of the short-ids approach to monitoring. While we are no longer sending the larger payloads, we are still allocating and authoring them, which we should still avoid doing if we can avoid it. The monitoring method is what needs changing to "understand" the capabilities and provide one thing or another (or if we have no information, both).
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #17821: Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1145394799
Run Go PostCommit
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #17821: [BEAM-14557] Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1147675402
The portable and flink runners are not requesting `req.MonitoringInfos()`. That's the reason they are not getting any monitoring infos with the new approach.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck merged pull request #17821: [BEAM-14557] Read and Seek Runner Capabilities in Go SDK
Posted by GitBox <gi...@apache.org>.
lostluck merged PR #17821:
URL: https://github.com/apache/beam/pull/17821
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #17821: Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1145377903
R: @lostluck
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on a diff in pull request #17821: [BEAM-14557] Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #17821:
URL: https://github.com/apache/beam/pull/17821#discussion_r894029114
##########
sdks/go/pkg/beam/core/runtime/graphx/translate.go:
##########
@@ -86,7 +86,7 @@ const (
func goCapabilities() []string {
capabilities := []string{
- URNLegacyProgressReporting,
+ URNProgressReporting,
Review Comment:
It looks like "beam:protocol:monitoring_info_short_ids:v1" is the one we should be providing and looking for, which is why we aren't seeing anything on the Worker side. This should resolve it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #17821: [BEAM-14557] Remove monitoring infos
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #17821:
URL: https://github.com/apache/beam/pull/17821#issuecomment-1152680875
Since the scope of the PR has changed, could the Title and PR Description be updated accordingly?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org