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/08/23 15:50:15 UTC
[GitHub] [beam] jrmccluskey opened a new pull request, #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
jrmccluskey opened a new pull request, #22824:
URL: https://github.com/apache/beam/pull/22824
Adds support for the Google Cloud Profiler to the Go SDK through the `enable_google_cloud_profiler` option similar to Java and Python. Handles getting job name/id information at pipeline run time through metadata. Validated with custom worker container and streaming pipeline, as short-running jobs like wordcount will not produce output.
------------------------
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`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] 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/get-started-contributing/#make-the-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)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+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] lostluck commented on a diff in pull request #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #22824:
URL: https://github.com/apache/beam/pull/22824#discussion_r954299134
##########
sdks/go/container/boot.go:
##########
@@ -121,6 +128,25 @@ func main() {
os.Setenv("RUNNER_CAPABILITIES", strings.Join(info.GetRunnerCapabilities(), " "))
}
+ enableGoogleCloudProfiler := strings.Contains(options, enableGoogleCloudProfilerOption)
+ if enableGoogleCloudProfiler {
+ if metadata := info.GetMetadata(); metadata != nil {
+ if jobName, nameExists := metadata["job_name"]; nameExists {
+ if jobId, idExists := metadata["job_id"]; idExists {
+ os.Setenv(cloudProfilingJobName, jobName)
+ os.Setenv(cloudProfilingJobID, jobId)
+ log.Printf("Cloud Profiling Job Name: %v, Job IDL %v", jobName, jobId)
+ } else {
+ log.Println("Required job_id missing from metadata, profiling will not be enabled without it.")
+ }
+ } else {
+ log.Println("Required job_name missing from metadata, profiling will not be enabled without it.")
+ }
+ } else {
+ log.Println("enable_google_cloud_profiler is set to true, but no metadata is received from provision server, profiling will not be enabled.")
+ }
+ }
Review Comment:
Style wise, it's better to not have the deepening if-nesting.
I recommend moving this logic into a function, and test that function. Moving it into a function keeps main cleaner, and lets the logic be more linear too, with less nesting.
eg.
```
if enableGoogleCloudProfiler {
if err := configureGoogleCloudProfilerEnvVars(info.GetMetadata()); err != nil {
log.Println(err)
}
}
```
And then the method looks something like:
```
func configureGoogleCloudProfilerEnvVars(metadata map[string]string) error {
if metadata == nil {
return fmt.Errorf("enable_google_cloud_profiler is set to true, but no metadata is received from provision server, profiling will not be enabled.")
}
jobName, nameExists := metadata["job_name"]
if !nameExists {
return fmt.Errorf("required job_name missing from metadata, profiling will not be enabled without it.")
}
jobId, idExists := metadata["job_id"];
if !idExists {
return fmt.Errorf("Required job_id missing from metadata, profiling will not be enabled without it.")
}
os.Setenv(cloudProfilingJobName, jobName)
os.Setenv(cloudProfilingJobID, jobId)
log.Printf("Cloud Profiling Job Name: %v, Job IDL %v", jobName, jobId)
return nil
}
```
This keeps the "happy path" on the left edge, and avoids making the error messages be defined as far as possible from the error they're describing.
https://zchee.github.io/golang-wiki/CodeReviewComments/#indent-error-flow
Moving to a function also lets us return out immeadiately when there's a problem, avoiding the nested if-ladder.
This approach also makes it so we can unit test the configuration too, since we can check that we get an error if something is wrong, instead of trying to hack in testing the log messages directly.
Finally, this lets you define the constants adjacent to the relevant function, so they're all together.
--
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 #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22824:
URL: https://github.com/apache/beam/pull/22824#issuecomment-1235428623
Reminder, please take a look at this pr: @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] jrmccluskey commented on a diff in pull request #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on code in PR #22824:
URL: https://github.com/apache/beam/pull/22824#discussion_r955229634
##########
sdks/go/container/boot.go:
##########
@@ -121,6 +128,25 @@ func main() {
os.Setenv("RUNNER_CAPABILITIES", strings.Join(info.GetRunnerCapabilities(), " "))
}
+ enableGoogleCloudProfiler := strings.Contains(options, enableGoogleCloudProfilerOption)
+ if enableGoogleCloudProfiler {
+ if metadata := info.GetMetadata(); metadata != nil {
+ if jobName, nameExists := metadata["job_name"]; nameExists {
+ if jobId, idExists := metadata["job_id"]; idExists {
+ os.Setenv(cloudProfilingJobName, jobName)
+ os.Setenv(cloudProfilingJobID, jobId)
+ log.Printf("Cloud Profiling Job Name: %v, Job IDL %v", jobName, jobId)
+ } else {
+ log.Println("Required job_id missing from metadata, profiling will not be enabled without it.")
+ }
+ } else {
+ log.Println("Required job_name missing from metadata, profiling will not be enabled without it.")
+ }
+ } else {
+ log.Println("enable_google_cloud_profiler is set to true, but no metadata is received from provision server, profiling will not be enabled.")
+ }
+ }
Review Comment:
Done, reasonable change (and also potentially worth making in the other boot scripts too)
--
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] jrmccluskey commented on a diff in pull request #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on code in PR #22824:
URL: https://github.com/apache/beam/pull/22824#discussion_r955229257
##########
sdks/go.mod:
##########
@@ -52,6 +52,8 @@ require (
gopkg.in/yaml.v2 v2.4.0
)
+require cloud.google.com/go/profiler v0.3.0
Review Comment:
`go mod tidy` decided it was special enough for its own block, apparently. 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 a diff in pull request #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #22824:
URL: https://github.com/apache/beam/pull/22824#discussion_r954285428
##########
sdks/go.mod:
##########
@@ -52,6 +52,8 @@ require (
gopkg.in/yaml.v2 v2.4.0
)
+require cloud.google.com/go/profiler v0.3.0
Review Comment:
Consider putting this in the above `require` block instead of it's own line.
--
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] jrmccluskey merged pull request #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
jrmccluskey merged PR #22824:
URL: https://github.com/apache/beam/pull/22824
--
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 #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22824:
URL: https://github.com/apache/beam/pull/22824#issuecomment-1224378937
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @lostluck for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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 #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #22824:
URL: https://github.com/apache/beam/pull/22824#issuecomment-1226304290
Run GoPortable 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.
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 #22824: [Go SDK]: Add support for Google Cloud Profiler for pipelines
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22824:
URL: https://github.com/apache/beam/pull/22824#issuecomment-1224270292
# [Codecov](https://codecov.io/gh/apache/beam/pull/22824?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 [#22824](https://codecov.io/gh/apache/beam/pull/22824?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (950a167) into [master](https://codecov.io/gh/apache/beam/commit/dfa5ec58a192a35c20e3f54c9300fd611a98f7b0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dfa5ec5) will **decrease** coverage by `0.11%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #22824 +/- ##
==========================================
- Coverage 74.09% 73.98% -0.12%
==========================================
Files 712 713 +1
Lines 93832 94057 +225
==========================================
+ Hits 69526 69589 +63
- Misses 23026 23182 +156
- Partials 1280 1286 +6
```
| Flag | Coverage Δ | |
|---|---|---|
| go | `51.29% <0.00%> (-0.19%)` | :arrow_down: |
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/22824?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/22824/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.00% <0.00%> (-0.19%)` | :arrow_down: |
| [sdks/go/pkg/beam/core/runtime/harness/statemgr.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvaGFybmVzcy9zdGF0ZW1nci5nbw==) | `46.59% <0.00%> (-5.06%)` | :arrow_down: |
| [sdks/go/pkg/beam/core/state/state.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3N0YXRlL3N0YXRlLmdv) | `74.35% <0.00%> (-4.02%)` | :arrow_down: |
| [sdks/go/pkg/beam/core/runtime/exec/translate.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy90cmFuc2xhdGUuZ28=) | `13.67% <0.00%> (-0.25%)` | :arrow_down: |
| [sdks/go/pkg/beam/core/runtime/exec/pardo.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9wYXJkby5nbw==) | `59.43% <0.00%> (ø)` | |
| [sdks/go/pkg/beam/core/runtime/exec/userstate.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy91c2Vyc3RhdGUuZ28=) | `16.96% <0.00%> (ø)` | |
| [sdks/go/pkg/beam/core/graph/fn.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL2dyYXBoL2ZuLmdv) | `84.55% <0.00%> (+0.55%)` | :arrow_up: |
| [sdks/go/pkg/beam/core/runtime/exec/coder.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9jb2Rlci5nbw==) | `59.02% <0.00%> (+0.80%)` | :arrow_up: |
| [sdks/go/pkg/beam/core/runtime/exec/fn.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9mbi5nbw==) | `68.32% <0.00%> (+0.83%)` | :arrow_up: |
| [sdks/go/pkg/beam/core/runtime/exec/decode.go](https://codecov.io/gh/apache/beam/pull/22824/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-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9kZWNvZGUuZ28=) | `81.39% <0.00%> (+6.97%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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