Respect end-tracing script instead of deleting one variable
#938
Conversation
edoardopirovano
force-pushed
the
respect-end-tracing
branch
2 times, most recently
from
3f68d85
to
84ab7c2
Compare
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
edoardopirovano
force-pushed
the
respect-end-tracing
branch
from
84ab7c2
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?
edoardopirovano
force-pushed
the
respect-end-tracing
branch
from
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