From f88bfbf3d15366f34e2b7c85566e15313fb7ec11 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Fri, 9 Jun 2023 16:09:21 -0400 Subject: [PATCH 1/4] Use legacy AppInsights to get better control over logging --- .gitignore | 1 - CHANGELOG.md | 6 +++--- project/Dependencies.scala | 4 ++-- server/src/main/resources/applicationinsights.json | 7 ------- server/src/main/resources/logback.xml | 3 +++ .../src/main/scala/cromwell/CromwellEntryPoint.scala | 12 ------------ 6 files changed, 8 insertions(+), 25 deletions(-) delete mode 100644 server/src/main/resources/applicationinsights.json diff --git a/.gitignore b/.gitignore index d67639e9ef1..a5b72f6b263 100644 --- a/.gitignore +++ b/.gitignore @@ -55,4 +55,3 @@ tesk_application.conf **/venv/ exome_germline_single_sample_v1.3/ **/*.pyc -applicationinsights.log diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a7c097699e..526c244da03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,9 +6,9 @@ WDL `size` engine function now works for HTTP files. -### Azure Instrumentation Support -Cromwell can now send logs and process information to Azure Application Insights. To enable, set environment -variable `APPLICATIONINSIGHTS_CONNECTION_STRING` to your connection string. [See here for information.](https://learn.microsoft.com/en-us/azure/azure-monitor/app/sdk-connection-string) +### Azure ApplicationInsights Logging Support +Cromwell can now send logs to Azure Application Insights. To enable, set environment +variable `APPLICATIONINSIGHTS_INSTRUMENTATIONKEY` to your account's key. [See here for information.](https://learn.microsoft.com/en-us/azure/azure-monitor/app/sdk-connection-string) ## 85 Release Notes diff --git a/project/Dependencies.scala b/project/Dependencies.scala index b5fad373834..5f12591f523 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -12,7 +12,7 @@ object Dependencies { // https://github.com/sbt/sbt/issues/4531 private val azureStorageBlobNioV = "12.0.0-beta.19" private val azureIdentitySdkV = "1.9.0-beta.2" - private val azureAppInsightsV = "3.4.12" + private val azureAppInsightsLogbackV = "2.6.4" private val betterFilesV = "3.9.1" private val jsonSmartV = "2.4.10" /* @@ -213,7 +213,7 @@ object Dependencies { "com.fasterxml.jackson.dataformat" % "jackson-dataformat-xml" % jacksonV, "com.azure.resourcemanager" % "azure-resourcemanager" % "2.18.0", "net.minidev" % "json-smart" % jsonSmartV, - "com.microsoft.azure" % "applicationinsights-runtime-attach" % azureAppInsightsV, + "com.microsoft.azure" % "applicationinsights-logging-logback" % azureAppInsightsLogbackV, ) val wsmDependencies: List[ModuleID] = List( diff --git a/server/src/main/resources/applicationinsights.json b/server/src/main/resources/applicationinsights.json deleted file mode 100644 index e90407242bf..00000000000 --- a/server/src/main/resources/applicationinsights.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "instrumentation": { - "jdbc": { - "enabled": false - } - } -} diff --git a/server/src/main/resources/logback.xml b/server/src/main/resources/logback.xml index 006ca3953f4..29582f140d3 100644 --- a/server/src/main/resources/logback.xml +++ b/server/src/main/resources/logback.xml @@ -94,11 +94,14 @@ + + + diff --git a/server/src/main/scala/cromwell/CromwellEntryPoint.scala b/server/src/main/scala/cromwell/CromwellEntryPoint.scala index 9f6ce82a127..d712ea71b28 100644 --- a/server/src/main/scala/cromwell/CromwellEntryPoint.scala +++ b/server/src/main/scala/cromwell/CromwellEntryPoint.scala @@ -9,7 +9,6 @@ import cats.data.Validated._ import cats.effect.{ContextShift, IO} import cats.syntax.apply._ import cats.syntax.validated._ -import com.microsoft.applicationinsights.attach.ApplicationInsights import com.typesafe.config.{Config, ConfigFactory} import common.exception.MessageAggregation import common.validation.ErrorOr._ @@ -46,11 +45,6 @@ object CromwellEntryPoint extends GracefulStopSupport { private val dnsCacheTtl = config.getOrElse("system.dns-cache-ttl", 3 minutes) java.security.Security.setProperty("networkaddress.cache.ttl", dnsCacheTtl.toSeconds.toString) - // The presence of this env var tells us that the user is trying to send instrumentation data and - // logs to Azure Application Insights. If it's present, we'll attach the ApplicationInsights agent. - // To configure the behavior of this agent, see server/src/main/resources/applicationinsights.json - private lazy val useAzureInstrumentation = sys.env.contains("APPLICATIONINSIGHTS_CONNECTION_STRING") - /** * Run Cromwell in server mode. */ @@ -151,12 +145,6 @@ object CromwellEntryPoint extends GracefulStopSupport { */ private def initLogging(command: Command): Unit = { - // Enable Azure instrumentation and log-slurping if desired. Running ApplicationInsights.attach() - // without checking for the presence of the relevant env var doesn't cause any failures, but does - // print an error that would be confusing for non-Azure users. - if (useAzureInstrumentation) - ApplicationInsights.attach() - val logbackSetting = command match { case Server => "STANDARD" case Submit => "PRETTY" From a25678e48b326457aeeebede93100a881208af6e Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Mon, 12 Jun 2023 14:00:51 -0400 Subject: [PATCH 2/4] Fix build --- project/Merging.scala | 3 +++ 1 file changed, 3 insertions(+) diff --git a/project/Merging.scala b/project/Merging.scala index 046380a367c..a84b6f5a4a5 100644 --- a/project/Merging.scala +++ b/project/Merging.scala @@ -9,6 +9,9 @@ object Merging { MergeStrategy.filterDistinctLines case PathList(ps@_*) if ps.last == "logback.xml" => MergeStrategy.first + // Merge mozilla/public-suffix-list.txt if duplicated + case PathList(ps@_*) if ps.last == "public-suffix-list.txt" => + MergeStrategy.first // AWS SDK v2 configuration files - can be discarded case PathList(ps@_*) if Set("codegen.config" , "service-2.json" , "waiters-2.json" , "customization.config" , "examples-1.json" , "paginators-1.json").contains(ps.last) => MergeStrategy.discard From 5570f48ea4721dcdff5a498addbd2741fda7dcb9 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Thu, 15 Jun 2023 14:58:23 -0400 Subject: [PATCH 3/4] Add comment explaining version choice --- project/Dependencies.scala | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 5f12591f523..d90ab1956c0 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -12,6 +12,11 @@ object Dependencies { // https://github.com/sbt/sbt/issues/4531 private val azureStorageBlobNioV = "12.0.0-beta.19" private val azureIdentitySdkV = "1.9.0-beta.2" + // We are using the older AppInsights 2 because we want to use the + // logback appender to send logs. AppInsights 3 does not have a standalone + // appender, and its auto-hoovering of logs didn't meet our needs. + // (Specifically, the side-by-side root logger and workflow logger resulted in + // duplicate messages in AI. See WX-1122.) private val azureAppInsightsLogbackV = "2.6.4" private val betterFilesV = "3.9.1" private val jsonSmartV = "2.4.10" From 5b3f14c0cb615433b407e66339c4e20cbdd99851 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Tue, 20 Jun 2023 14:06:31 -0400 Subject: [PATCH 4/4] Does this work? --- project/Merging.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project/Merging.scala b/project/Merging.scala index a84b6f5a4a5..5ecd951014b 100644 --- a/project/Merging.scala +++ b/project/Merging.scala @@ -11,7 +11,7 @@ object Merging { MergeStrategy.first // Merge mozilla/public-suffix-list.txt if duplicated case PathList(ps@_*) if ps.last == "public-suffix-list.txt" => - MergeStrategy.first + MergeStrategy.last // AWS SDK v2 configuration files - can be discarded case PathList(ps@_*) if Set("codegen.config" , "service-2.json" , "waiters-2.json" , "customization.config" , "examples-1.json" , "paginators-1.json").contains(ps.last) => MergeStrategy.discard