Respect end-tracing script instead of deleting one variable - #938
edoardopirovano merged 2 commits into
Conversation
Sorry, something went wrong.
criemen
left a comment
There was a problem hiding this comment.
LGTM from my side, but a JS person should review as well.
I checked that for C#, end-tracing.json indeed contains (besides others)
"COR_ENABLE_PROFILING":null,"COR_PROFILER":null,"COR_PROFILER_PATH_64":null,"CORECLR_ENABLE_PROFILING":null,"CORECLR_PROFILER":null
Sorry, something went wrong.
| const endTracingEnvVariables: Map<string, string | null> = JSON.parse( | ||
| fs.readFileSync( | ||
| path.resolve( | ||
| config.dbLocation, | ||
| "temp/tracingEnvironment/end-tracing.json" | ||
| ), | ||
| "utf8" | ||
| ) | ||
| ); | ||
| for (const [key, value] of Object.entries(endTracingEnvVariables)) { | ||
| if (value !== null) { | ||
| process.env[key] = value; | ||
| } else { | ||
| delete process.env[key]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you wrap this in a try-catch and rethrow with a good error message if the file doesn't exist or is malformed?
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
Small suggestion, but I think it is important to help avoid potential problems with users' workflows.
Sorry, something went wrong.
|
Thanks @aeisenberg, I've added some more error handling! I note that it would be nice to have this for the start of tracing as well. The natural thing to do would be to have a utility function that can parse either file and contains all the error handling. Unfortunately, it's not quite that easy because the types of the return value are different as the end tracing file can contain null values to unset environment variables whereas the start tracing one cannot, so we'd have to change the types of quite a few things all around the code that use the start tracing file. I propose leaving that extension for a future PR since that seems to be growing the scope of this PR too much. |
Sorry, something went wrong.
This might fix an issue we've observed with the CLR tracing continuing to trace after we've supposedly stopped tracing. It also just seems the more correct thing to do anyway. The variable that was previously deleted (ODASA_TRACER_CONFIGURATION) is always unset by end-tracing, so we should only be doing more to try and halt tracing.
Merge / deployment checklist