-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add instrument.sh script #1354
Add instrument.sh script #1354
Conversation
Co-authored-by: Paulo Janotti <[email protected]>
> On macOS [`coreutils`](https://formulae.brew.sh/formula/coreutils) is required. | ||
|
||
```sh | ||
curl -fL https://raw.githubusercontent.com/open-telemetry/opentelemetry-dotnet-instrumentation/main/instrument.sh -O |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should point to the release artifact instead of to the actual source. Specially when main is not a release tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure as any Writer
(approver) can change/remove/add release artifacts. The main
branch is extremely protected - requires GPG signatures, also only the maintainers can merge/push to `main. We can consider changing it to a "release tag" during the release process. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -172,6 +174,23 @@ CORECLR_PROFILER_PATH_64 | |||
OTEL_DOTNET_AUTO_INTEGRATIONS_FILE | |||
``` | |||
|
|||
You can also use the [instrument.sh](../instrument.sh) script which uses following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might consider breaking the "Instrument a .NET application" section into two smaller sections: 1) Environment variables and 2) Setup script. Based on my reading, that would help make sure someone who already sets the environment variables at the top doesn't try to run the instrument script when they don't have to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree. Tomorrow, I will create a separate PR to address it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one suggestion
Why
Create a script to help set up the auto-instrumentation. It should reduce the possibility of human errors and make it faster+easier to try out.
What
download.sh
instrument.sh
script to facilitate instrumenting .NET apps (for *nix)Tests
Checklist
CHANGELOG.md
is updated.