Skip to content

Always write to log on startup failure#1550

Merged
trask merged 2 commits into
masterfrom
trask/always-write-to-log-on-startup-failure
Mar 12, 2021
Merged

Always write to log on startup failure#1550
trask merged 2 commits into
masterfrom
trask/always-write-to-log-on-startup-failure

Conversation

@trask

@trask trask commented Mar 12, 2021

Copy link
Copy Markdown
Member

Also introduces env var APPLICATIONINSIGHTS_SELF_DIAGNOSTICS_FILE_PATH, which can also be helpful in this case for redirecting log file to another location.

// If the startupLogger still have some issues being initialized, just print the error stack trace
} catch (Throwable ignored) {
// this is a last resort in cases where the JVM doesn't have write permission to the directory where the agent lives
// and the user has not specified APPLICATIONINSIGHTS_SELF_DIAGNOSTICS_FILE_PATH

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

file.path is currently default to "applicationinsights.log". for azure function, it doesn't have the write permission in the agent jar folder. can we make azure function or other attaches to default to "/var/log/applicationinsights.log"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

of course, we can always ask them to provide a value for this env var or declare it in the json config.
this env var will have a higher precedence over the json config, right? similar to other env vars' precedence.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this shouldn't happen for users, since they can't control the json file, and so can't mess it up accidentally and cause it to fail to start

also will still log to /tmp/applicationinsights.log now which is better than before (just not the more ideal /var/log/applicationinsights.log, trying to balance the amount of special cases we add to the code for different compute environments)


@Test
public void shouldOverrideSelfDiagnosticsFilePath() throws IOException {
envVars.set("APPLICATIONINSIGHTS_SELF_DIAGNOSTICS_FILE_PATH", "/tmp/ai.log");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

before overriding, can we assertEquals the value from the applicationinsights.json?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the shouldParse test above does this

@trask trask merged commit 93657fd into master Mar 12, 2021
@trask trask deleted the trask/always-write-to-log-on-startup-failure branch March 12, 2021 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants