Respect end-tracing script instead of deleting one variable#938
Respect end-tracing script instead of deleting one variable#938edoardopirovano merged 2 commits intomainfrom
end-tracing script instead of deleting one variable#938Conversation
3f68d85 to
84ab7c2
Compare
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":null84ab7c2 to
6a910c7
Compare
src/tracer-config.ts
Outdated
| 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?
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.
6a910c7 to
afbddca
Compare
|
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 I propose leaving that extension for a future PR since that seems to be growing the scope of this PR too much. |
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 byend-tracing, so we should only be doing more to try and halt tracing.Merge / deployment checklist