[core-tracing] Allow tracing to be disabled by environment #5748
Conversation
| @@ -23,5 +23,8 @@ export function getTracer() { | |||
| if (!cache.tracer) { | |||
| cache.tracer = new NoOpTracer(); | |||
| } | |||
| if (process.env.AZURE_TRACING_DISABLED) { | |||
There was a problem hiding this comment.
Builds are failing in some places because of the use of process, I think event-hubs doesn't have its shim set up right. Might need to wrap this in a if (isNode) { block so that it gets elided in the browser builds.
There was a problem hiding this comment.
it seems like we were trying to check window.__env__ in the browser tho
There was a problem hiding this comment.
in the logger package we were able to pass process: false in package.json:
{
"browser": {
"./dist/index.js": "./browser/logger.js",
"./dist-esm/src/log.js": "./dist-esm/src/log.browser.js",
"process": false
},
}There was a problem hiding this comment.
maybe that will help with the build breaks?
There was a problem hiding this comment.
process: false in the package.json browser map, plus guarding the process.env check with isNode, is probably the right way to go. I don't think we should support environment variables in the browser.
|
Hey @chradek, how does this approach look to you for disabling tracing via environment variable? Is there any possible gotcha here? |
| @@ -23,5 +28,8 @@ export function getTracer() { | |||
| if (!cache.tracer) { | |||
| cache.tracer = new NoOpTracer(); | |||
| } | |||
| if (env.AZURE_TRACING_DISABLED) { | |||
There was a problem hiding this comment.
it does feel weird that this doesn't have to be truthy at all, e.g. someone could set AZURE_TRACING_DISABLED=false or AZURE_TRACING_DISABLED=0 and it would still disable
| @@ -23,5 +28,8 @@ export function getTracer() { | |||
| if (!cache.tracer) { | |||
| cache.tracer = new NoOpTracer(); | |||
| } | |||
| if (env.AZURE_TRACING_DISABLED) { | |||
| cache.tracer = new NoOpTracer(); | |||
There was a problem hiding this comment.
getTracer gets called a lot, but right now you'd create a new no-op tracer each time... feels like we should maybe just have a static noop tracer that you can return early with instead of making a new one
This allows tracing to be disabled via the AZURE_TRACING_DISABLED environment variable.
Part of the implementation for #5696.