Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

use regular thread for to expose errors in appengine #1955

Merged

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Jul 29, 2019

This fixes #1954 for OC+GAE (jdk8/jdk11).

Solution

Use normal thread for GAE with jdk8/jdk11. Logs generated by any thread not associated with any request are stored in memory. These logs are flushed when a next request is served.

Caveat

If no request is served or if the request is serve long after the message is logged then the message may not appear in the appengine logs. In most practical scenario, the QPS will be sufficient enough to flush these pending log messages.

@rghetia rghetia requested review from dinooliva, songy23 and a team as code owners July 29, 2019 23:40
@@ -39,7 +39,9 @@

private IntervalMetricReader(Worker worker) {
this.worker = worker;
this.workerThread = new DaemonThreadFactory().newThread(worker);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change? It looks to me nothing changed for JDK 8+. The previous approach only restricts JDK 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous approach created thread using MoreExecutors.platformThreadFactory() for all JDK. We just need regular thread for logging to work.

try {
thread.setName(threadPrefix + threadIdGen.getAndIncrement());
thread.setDaemon(true);
} catch (SecurityException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to remove this try-catch clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constraints on thread renaming is from AppEngine threads. Since we are not using them it should be okay. But let me put it back as it doesn't hurt to have try-catch clause.

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@songy23
Copy link
Contributor

songy23 commented Jul 30, 2019

Appveyor failure is not related to this change:

FAILURE: Build failed with an exception.
* What went wrong:
Could not resolve all files for configuration ':opencensus-contrib-log-correlation-log4j2:compileClasspath'.
> Could not resolve org.apache.logging.log4j:log4j-core:2.11.1.
  Required by:
      project :opencensus-contrib-log-correlation-log4j2
   > Could not resolve org.apache.logging.log4j:log4j-core:2.11.1.
      > Could not get resource 'https://repo.maven.apache.org/maven2/org/apache/logging/log4j/log4j-core/2.11.1/log4j-core-2.11.1.pom'.
         > Could not GET 'https://repo.maven.apache.org/maven2/org/apache/logging/log4j/log4j-core/2.11.1/log4j-core-2.11.1.pom'.
            > Connect to repo.maven.apache.org:443 [repo.maven.apache.org/151.101.184.215] failed: Connection timed out: connect

@songy23 songy23 merged commit b30318d into census-instrumentation:master Jul 30, 2019
@rghetia rghetia deleted the change_appengine_thread branch July 30, 2019 19:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error logs not generated by IntervalMetricsReader in GAE are not logged
3 participants