Adds CloudSdk parameter for setting structured logs value #426
Conversation
etanshaul
changed the title
etanshaul
changed the title
Codecov Report
@@ Coverage Diff @@
## master #426 +/- ##
==========================================
+ Coverage 65.13% 65.78% +0.64%
==========================================
Files 64 64
Lines 1761 1768 +7
Branches 272 274 +2
==========================================
+ Hits 1147 1163 +16
+ Misses 499 485 -14
- Partials 115 120 +5
Continue to review full report at Codecov.
|
|
this environment variable has always been present in cloud sdk? |
|
No it was added as part of the structured error changes. It made the 159 release. |
|
Do we need to update the minimum cloud sdk version? |
|
No, not necessarily. Setting this env var on an earlier version of the SDK will have no effect. |
| * Sets structured json logs for the stderr output. Supported values include 'never' (default), | ||
| * 'always', 'terminal', etc. | ||
| */ | ||
| public Builder appCommandShowStructuredLogs(String appCommandShowStructuredLogs) { |
There was a problem hiding this comment.
Name is unwieldy. Maybe just "useStructuredLogs"?
There was a problem hiding this comment.
the name mimics the gcloud config var name show_structured_logs. This is the patter we've been using here for the other parameters. But of course this can be debated.
There was a problem hiding this comment.
I agree the name isn't the best though; useStructuredLogs doesn't help much though because if you look at the values it accepts, the name should be something like setStructuredLogsScope or something like that
There was a problem hiding this comment.
Given that, I agree that "show" beats "use". However, I still think "appCommand" is unnecessary.
There was a problem hiding this comment.
the appCommand prefix, while ugly, is to show clearly that the scope of the parameter is for gcloud command execution (e.g. CloudSdk#runAppCommand as opposed to a dev server command or appcfg etc. which are also executed from the CloudSdk class
I'm not denying though that this whole thing could be refactored to avoid this, but I don't think this PR is the place for it
There was a problem hiding this comment.
actually it looks @loosebazooka has some code in the experimental package that addresses all of this.
|
hmm. If a client wants to show structured logs it's likely because they want to parse them. I'm not really a big fan of the parameter silently being ignored dependent on the user's Cloud SDK version. We have a pretty good notification UI for when the Cloud SDK needs to be updated for our plugins to work, I'd be OK with forcing them to come up to the version we need. |
|
I agree we should update the minimum SDK version. |
|
I'm looking into how I can add unit tests for this. |
| String appCommandOutputFormat, ProcessRunner processRunner, | ||
| @Nullable String appCommandOutputFormat, | ||
| @Nullable String appCommandShowStructuredLogs, | ||
| ProcessRunner processRunner, |
There was a problem hiding this comment.
Is this Nullable? I don't see a null check in the constructor. Ditto sdkPath and javaHomePath.
There was a problem hiding this comment.
they are not nullable
There was a problem hiding this comment.
actually they may be nullable depending on which command is invoked..
fixes #425
This new parameter mimics the new gcloud configuration for providing the structured error logs environment variable setting
CLOUDSDK_CORE_SHOW_STRUCTURED_LOGS.gcloud accepts a string value for this:
terminal,log,always,neverSetting this to terminal, or always, produces json output for messages coming from the cloudsdk according to the verbosity value currently set.
Example (consumed from IntelliJ):
Without structured logs set (defaulted to
never):Same error, with verbosity defaulted to
error, and the structure logs parameter set tolog: