Respect value of LD_PRELOAD given by the CLI - #817
edoardopirovano merged 1 commit into
Conversation
Sorry, something went wrong.
| ); | ||
| } | ||
| mainTracerConfig = concatTracerConfigs(tracedLanguageConfigs, config); | ||
| } |
There was a problem hiding this comment.
I love how this PR is just moving a brace, but an extremely important one!
This does make me think we should factor the else block into a getLegacyTracerConfig function so that it's easy to spot what's going on. No need in this PR though.
Sorry, something went wrong.
There was a problem hiding this comment.
Also it's good that the internal integration tests caught this, but why didn't the external tests catch this for 2.7.1?
Sorry, something went wrong.
There was a problem hiding this comment.
Agree that refactoring this might be sensible - let's leave that for later though. The tests against the nightly build will start failing soon if this isn't merged and I don't want this to hold up finishing off the 2.7.1 release tomorrow with a merge to v1.
2.7.1 did not include the renaming of the tracing mechanisms which only went into main earlier today and will end up in 2.7.2. So, with 2.7.1 the value of the LD_PRELOAD wasn't really being overwritten since it was just replaced with itself.
Sorry, something went wrong.
There was a problem hiding this comment.
That is a relief. It was overwritten but the value was the same, and we changed that value in 2.7.2. Thank you for explaining!
Sorry, something went wrong.
When implementing its own version of sandwiched tracing, the Action was setting a value of LD_PRELOAD. This code stayed outside of the condition for using the new style of tracing (i.e. using sandwiched tracing form the CLI). So, the Action would try to overwrite the value of LD_PRELOAD given by the CLI. This should not have been the case - when using the CLI's sandwiched tracing we should use the value of LD_PRELOAD provided by the CLI so that the CLI is free to move/rename the libraries as it pleases.
Merge / deployment checklist