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

Use application base directory to locate self diagnostic config file #1865

Merged
merged 5 commits into from
Mar 3, 2021

Conversation

tbolon
Copy link
Contributor

@tbolon tbolon commented Mar 3, 2021

Fixes #1714. Invalidate #1717

Changes

This change updates the way the self diagnostic config file is located. Instead of using environment current directory, it uses the application base directory.

This fixes self diagnostic config file lookup in ASP.NET applications and makes the app compatible with all kinds of hosting.

This PR should also update documentation (readme).

@tbolon tbolon requested a review from a team March 3, 2021 14:26
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 3, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #1865 (38c1dc7) into main (cb066cb) will increase coverage by 0.22%.
The diff coverage is 99.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1865      +/-   ##
==========================================
+ Coverage   83.77%   84.00%   +0.22%     
==========================================
  Files         187      187              
  Lines        5967     5988      +21     
==========================================
+ Hits         4999     5030      +31     
+ Misses        968      958      -10     
Impacted Files Coverage Δ
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 71.42% <ø> (ø)
src/OpenTelemetry/Trace/TracerProviderSdk.cs 95.16% <98.64%> (+4.38%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 86.89% <100.00%> (+0.05%) ⬆️
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 89.10% <100.00%> (ø)
...entation.AspNet/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...umentation.AspNetCore/AspNetCoreInstrumentation.cs 100.00% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 86.56% <100.00%> (ø)
...tion.AspNetCore/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...ntation.GrpcNetClient/GrpcClientInstrumentation.cs 100.00% <100.00%> (+12.50%) ⬆️
... and 14 more

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

I think this can be an option but not the only option.
In many environments the application does not have write permission to its base directory.
Please consider this:

  • User the "current working directory".
  • If there is no configuration file in the "current working directory", use the application image base directory.

@tbolon
Copy link
Contributor Author

tbolon commented Mar 3, 2021

It's not the app which is supposed to write on its base directory, but the user who wants to enable diagnostics.

This change only change the location scanned for the self diagnostic config file. It's expected that you (deployment, admin, etc.) can write config files on the app directory. Of course, the config itself should point to a directory where the app can write logs.

Currently, the location scanned is the current directory, which in most (all ?) cases the base directory. Most .NET apps are considering that config files are located along the binaries. Even for asp.net core apps.

Only in IIS, the w3wp process is setting c:\Windows\system32\inetsrv as the current directory. Which means all sites on your server will scan this single directory, and adding the config file on this path (which requires an elevated prompt) will enable diagnostics for all sites hosted on IIS.

It could be interesting to check how asp.net core itself determines where the web.config is located. (or appsettings.json for asp.net core). I think adopting a similar logic would be sufficent (instead of learning about a new location convention).

@reyang
Copy link
Member

reyang commented Mar 3, 2021

I think you are mistaking the self diagnostic config file and the log files: this change only fixes the location of the diagnostic config file. It's expected that you (deployment, admin, etc.) can write config files on the app directory. Of course, the file itself should point to a directory where the app can write logs.

I thought I mentioned "configuration file" in comment 😄
In many cases, the application base directory is "locked down" after deployment, and the application instances (processes) are running on separate folder for each user.

@tbolon
Copy link
Contributor Author

tbolon commented Mar 3, 2021

I think you are mistaking the self diagnostic config file and the log files: this change only fixes the location of the diagnostic config file. It's expected that you (deployment, admin, etc.) can write config files on the app directory. Of course, the file itself should point to a directory where the app can write logs.
Currently, the location scanned is the current directory, which in most case is the same as the base directory. Only in IIS, the w3wp process is using c:\Windows\system32\inetsrv as the current directory. Which means all sites on your server will scan this single directory, and adding the config file on this path (which requires an elevated prompt) will enable diagnostics for all sites hosted on IIS.

I thought I mentioned "configuration file" in comment 😄
In many cases, the application base directory is "locked down" after deployment, and the application instances (processes) are running on separate folder for each user.

Sorry, I edited my reply just now because your comment was clear, and I misunderstood it.

You have a good point about frozen deployments. I will update the code based on your suggestion.

@tbolon
Copy link
Contributor Author

tbolon commented Mar 3, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM (need to wait for the CLA though).
Please update the CHANGELOG.

@tbolon
Copy link
Contributor Author

tbolon commented Mar 3, 2021

LGTM (need to wait for the CLA though).

I opened a ticket on the LFX jira.

Done, I was able to sign the CLA

Please update the CHANGELOG.

I suggest to wait for the #1717 PR to be merged, with a generic comment about asp.net current state. Then I could rebase my PR to explain the changes? sorry, I read README 😅

@tbolon
Copy link
Contributor Author

tbolon commented Mar 3, 2021

I have tested the effets of my PR using AppContext.BaseDirectory with dotnet run and starting the exe directly: both returns the full path to bin\Debug\netcoreapp3.1\.

It means that the readme changes could also be simplified to say that in most cases, you could just drop the file along your application. And in the case where the base directory is not writable, you can use the directory returned by Environment.GetCurrentDirectory() if different.

@tbolon
Copy link
Contributor Author

tbolon commented Mar 3, 2021

Please update the CHANGELOG.

done

@cijothomas cijothomas merged commit 7da935e into open-telemetry:main Mar 3, 2021
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.

SDK does not output logs for ASP.Net Application
3 participants