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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
