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

Add eval at the end of the instrument script #1681

Closed

Conversation

dszmigielski
Copy link
Contributor

Why

Instrument script only sets env variables, but does not execute instrumented app.

What

Add eval at the end of the script

@pellared
Copy link
Member

Can you describe the use case? Currently the docs instructs to use it to export the needed env vars. Will . instrument.sh still work?

@dszmigielski
Copy link
Contributor Author

Can you describe the use case? Currently the docs instructs to use it to export the needed env vars. Will . instrument.sh still work?

It seems I was using that in a way it was not supposed to be used. Should I just close it or can it be considered a good change if it doesn't break the previous behaviour (. instrument.sh)?

@pellared
Copy link
Member

pellared commented Nov 28, 2022

I prefer closing it. I think "eval" should be avoided. People can always create "subshells" e.g.

dotnet publish -o out
(
  . $HOME/.otel-dotnet-auto/instrument.sh
  export OTEL_SERVICE_NAME=myapp
  export OTEL_RESOURCE_ATTRIBUTES=deployment.environment=staging,service.version=1.0.0
  ./out/myapp
) 

pjanotti
pjanotti previously approved these changes Nov 29, 2022
@pjanotti
Copy link
Contributor

We also have a script with the same name under dev/. The run-example.sh script uses the one under dev/ perhaps something to review when doing #1388

@pjanotti pjanotti mentioned this pull request Nov 29, 2022
@pjanotti pjanotti dismissed their stale review November 29, 2022 00:20

Missed @pellared comment about intended usage for this script

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.

3 participants