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.

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

Added support to read diagnostics config from the path defined in environment variable #2769

Merged
merged 14 commits into from
Oct 17, 2023

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Apr 18, 2023

Fix Issue #2724.

Changes

Design assumption:
Function Apps environment will set the value of this environment variable: APPLICATIONINSIGHTS_LOG_DIAGNOSTICS to be a directory that

  1. Users can create/delete diagnostics file and,
  2. SDK has the privilege to write to
    at host initialization.

The SDK reads the value of APPLICATIONINSIGHTS_LOG_DIAGNOSTICS, and parse settings of ApplicationInsightsDiagnostics.json file and write diagnostics log to the directory.

Checklist

  • I ran Unit Tests locally.
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

  • FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

@Yun-Ting Yun-Ting marked this pull request as ready for review April 19, 2023 22:39
@Yun-Ting Yun-Ting requested a review from xiang17 April 19, 2023 22:52
Copy link
Member

@xiang17 xiang17 left a comment

Choose a reason for hiding this comment

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

LGTM

configFilePath = Path.Combine(logDiagnosticsPath, ConfigFileName);
logDirectory = logDiagnosticsPath;
}

Copy link
Member

Choose a reason for hiding this comment

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

remove the blank space before else if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want to have a blank line before comment?

…tionInsights.Tests/Extensibility/Implementation/Tracing/SelfDiagnostics/SelfDiagnosticsConfigRefresherTest.cs

Co-authored-by: Reiley Yang <reyang@microsoft.com>
@cijothomas
Copy link
Contributor

Function Apps environment will set the value of this environment variable: APPLICATIONINSIGHTS_LOG_DIAGNOSTICS to be a directory that

Users can create/delete diagnostics file and,
SDK has the privilege to write to
at host initialization.
The SDK reads the value of APPLICATIONINSIGHTS_LOG_DIAGNOSTICS, and parse settings of ApplicationInsightsDiagnostics.json file and write diagnostics log to the directory.

Bit confused with this description.
The following is my understanding:
The SDK needs read permission for the directory specified in APPLICATIONINSIGHTS_LOG_DIAGNOSTICS.
The SDK needs write permission for the directory specified in ApplicationInsightsDiagnostics.json, it could be different directory than specified in APPLICATIONINSIGHTS_LOG_DIAGNOSTICs.

@Yun-Ting
Copy link
Contributor Author

it could be different directory than specified in APPLICATIONINSIGHTS_LOG_DIAGNOSTICs.

@cijothomas :
Not really. Both the ApplicationInsightsDiagnostics.json and the log file, for example: 20010101-120000.foobar.exe.12345.log will be placed in the directory that was set as the value of the environment variable of APPLICATIONINSIGHTS_LOG_DIAGNOSTICS.

See:

@TimothyMothra
Copy link
Member

@Yun-Ting please add an entry in the changelog.

@Yun-Ting Yun-Ting changed the title Added support to read diagnostics config from the path defined in environment variable. Added support to read diagnostics config from the path defined in environment variable on Windows. Oct 12, 2023
@Yun-Ting Yun-Ting changed the title Added support to read diagnostics config from the path defined in environment variable on Windows. Added support to read diagnostics config from the path defined in environment variable Oct 17, 2023
@Yun-Ting Yun-Ting merged commit 0f0c876 into main Oct 17, 2023
59 checks passed
@Yun-Ting Yun-Ting deleted the yunl/funcApp3 branch October 17, 2023 22:41
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.

None yet

5 participants