Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WX-1122 Use legacy AppInsights to get better control over logging #7157

Merged
merged 6 commits into from
Jun 21, 2023
Merged

WX-1122 Use legacy AppInsights to get better control over logging #7157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f88bfbf
Use legacy AppInsights to get better control over logging
jgainerdewar Jun 9, 2023
a25678e
Fix build
jgainerdewar Jun 12, 2023
f71a9bf
Merge branch 'develop' into jgd_WX-1122_legacyAppInsights
jgainerdewar Jun 15, 2023
5570f48
Add comment explaining version choice
jgainerdewar Jun 15, 2023
5b3f14c
Does this work?
jgainerdewar Jun 20, 2023
28c0822
Merge branch 'develop' into jgd_WX-1122_legacyAppInsights
jgainerdewar Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,3 @@ tesk_application.conf
**/venv/
exome_germline_single_sample_v1.3/
**/*.pyc
applicationinsights.log
6 changes: 3 additions & 3 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions project/Dependencies.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@ 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"
// 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"
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 want to leave a comment here warning the future upgrade-hungry developer about consequences of bumping this version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good idea.

private val betterFilesV = "3.9.1"
private val jsonSmartV = "2.4.10"
/*
Expand Down Expand Up @@ -213,7 +218,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(
Expand Down
3 changes: 3 additions & 0 deletions project/Merging.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.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
Expand Down
7 changes: 0 additions & 7 deletions server/src/main/resources/applicationinsights.json
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions server/src/main/resources/logback.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,14 @@
</filter>
</appender>

<appender name="AppInsightsAppender" class="com.microsoft.applicationinsights.logback.ApplicationInsightsAppender" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that Sentry is in here is seemed appropriate to include this.


<root level="${LOG_LEVEL}">
<appender-ref ref="STANDARD_APPENDER" />
<appender-ref ref="PRETTY_APPENDER" />
<appender-ref ref="FILEROLLER_APPENDER" />
<appender-ref ref="Sentry" />
<appender-ref ref="AppInsightsAppender" />
</root>

<logger name="liquibase" level="WARN"/>
Expand Down
12 changes: 0 additions & 12 deletions server/src/main/scala/cromwell/CromwellEntryPoint.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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._
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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"
Expand Down