-
Notifications
You must be signed in to change notification settings - Fork 41
adding custom logging test + local runtimes-common integration test #134
Changes from 1 commit
991e696
d1ea344
d2aa233
bf08154
5e98cd7
649b8d2
b05354e
0cffb44
ff6b6cf
e3eff4c
1654ef9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
<maven.compiler.source>${java.version}</maven.compiler.source> | ||
<maven.compiler.target>${java.version}</maven.compiler.target> | ||
<deployment.token>default_deployment_token</deployment.token> | ||
<spring.profile>gcp</spring.profile> | ||
</properties> | ||
|
||
<dependencies> | ||
|
@@ -42,6 +43,16 @@ | |
<artifactId>google-auth-library-credentials</artifactId> | ||
<version>0.7.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>com.google.cloud</groupId> | ||
<artifactId>google-cloud-logging</artifactId> | ||
<version>1.2.1</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.mockito</groupId> | ||
<artifactId>mockito-core</artifactId> | ||
<version>2.8.47</version> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
</dependency> | ||
</dependencies> | ||
|
||
<build> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
package com.google.cloud.runtimes; | ||
|
||
import com.google.cloud.MonitoredResource; | ||
import com.google.cloud.logging.LogEntry; | ||
import com.google.cloud.logging.Logging; | ||
import com.google.cloud.logging.Payload; | ||
import com.google.cloud.logging.Severity; | ||
import com.google.cloud.runtimes.stackdriver.StackDriverMonitoringService; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused import |
||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.beans.factory.annotation.Qualifier; | ||
import org.springframework.web.bind.annotation.RequestBody; | ||
import org.springframework.web.bind.annotation.RequestMapping; | ||
import org.springframework.web.bind.annotation.RestController; | ||
import sun.util.logging.resources.logging; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unusued import |
||
|
||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.logging.Logger; | ||
|
||
import static com.google.cloud.ServiceOptions.getDefaultProjectId; | ||
import static org.springframework.web.bind.annotation.RequestMethod.POST; | ||
|
||
@RestController | ||
public class LoggingTestController { | ||
|
||
@Autowired | ||
private Logging logging; | ||
@Autowired | ||
@Qualifier("projectId") | ||
private String projectId; | ||
|
||
private static Logger LOG = Logger.getLogger(LoggingTestController.class.getName()); | ||
|
||
public static class LoggingTestRequest { | ||
private String level; | ||
private String log_name; | ||
private String token; | ||
|
||
public void setLevel(String level) { | ||
this.level = level; | ||
} | ||
|
||
public void setLog_name(String log_name) { | ||
this.log_name = log_name; | ||
} | ||
|
||
public void setToken(String token) { | ||
this.token = token; | ||
} | ||
|
||
|
||
@Override | ||
public String toString() { | ||
return "LoggingTestRequest{" + | ||
"level='" + level + '\'' + | ||
", log_name='" + log_name + '\'' + | ||
", token='" + token + '\'' + | ||
'}'; | ||
} | ||
} | ||
|
||
@RequestMapping(path = "/logging_custom", method = POST) | ||
public String handleMonitoringRequest(@RequestBody LoggingTestRequest loggingTestRequest) throws IOException, InterruptedException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
LOG.info(String.valueOf(loggingTestRequest)); | ||
|
||
List<LogEntry> entries = new ArrayList<>(); | ||
Payload.StringPayload payload = Payload.StringPayload.of(loggingTestRequest.token); | ||
Severity severity = Severity.valueOf(loggingTestRequest.level); | ||
LogEntry entry = LogEntry.newBuilder(payload) | ||
.setSeverity(severity) | ||
.setLogName(loggingTestRequest.log_name) | ||
.setResource(MonitoredResource.newBuilder("global").build()) | ||
.build(); | ||
entries.add(entry); | ||
logging.write(entries); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using the Logging client lib directly, we should be able to use the JUL appender. Can you also look at #72 that has been put on the back-burner, and reconcile it with what you're doing in this PR? Ideally, we would like to close out #72 as well, but I think there are some unresolved complications there with setting up JUL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the Custom Logging Test should use the client library and make an explicit call to Stackdriver Logging (by creating only one LogEntry), whereas the Standard Logging Test should redirect the output of JUL to Stackdriver. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @cassand - I was planning to use JUL for standard logging tests and Logging library directly for custom, just so that we have both covered. If you're okay with that, I will review #72 in the context of the Standard Logging test (I just added a new, separate issue for it: #139) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm OK with Damien's suggestion. |
||
LOG.info("Log written to StackDriver: " + entries); | ||
return "OK"; | ||
} | ||
|
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package com.google.cloud.runtimes.config; | ||
|
||
import com.google.cloud.logging.Logging; | ||
import com.google.cloud.logging.LoggingOptions; | ||
import com.google.cloud.monitoring.v3.MetricServiceClient; | ||
import org.springframework.beans.factory.annotation.Qualifier; | ||
import org.springframework.context.annotation.Bean; | ||
import org.springframework.context.annotation.Configuration; | ||
import org.springframework.context.annotation.Profile; | ||
|
||
import java.io.IOException; | ||
|
||
import static com.google.cloud.ServiceOptions.getDefaultProjectId; | ||
|
||
@Configuration | ||
@Profile("gcp") | ||
public class GCPConfiguration { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
@Bean | ||
public Logging getLogging() { | ||
LoggingOptions options = LoggingOptions.getDefaultInstance(); | ||
return options.getService(); | ||
} | ||
|
||
@Bean | ||
public MetricServiceClient getMetricServiceClient() throws IOException { | ||
return MetricServiceClient.create(); | ||
} | ||
|
||
@Qualifier("projectId") | ||
@Bean | ||
public String getProjectId() { | ||
return getDefaultProjectId(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense to me to add these integration tests to
the local_integration_test.sh
script. I think we originally added this script in order to test things that were outside the scope of the runtimes_common integraion testing framework (namely, container shutdown hooks).I think if we wanted to make it easier to run the runtimes_common tests, a better way to do it would be to just simulate a cloudbuild execution locally, using the
container-builder-local
emulator that was just released as part of the Cloud SDK (see release notes).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooo, I'll test out this
container-build-local
, thanks for the suggestion!I created a separate script first btw, but then I figured that they were so similar that I integrated it back into the local_integration_test.sh. It's really handy to have as part of the workflow. But if container-build-local can do the magic, I'll have a look at that as that could remove the duplication around the arguments of the runtimes-common driver for example (one place here, the other place is the yaml file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do this in #126 though, and then separate it out from this pull request.