Clean Up Shim #1260
Clean Up Shim #1260
Conversation
| | webInstrumentationConfig | Not currently supported by the shim, but is in Azure Monitor OpenTelemetry as `browserSdkLoaderConfig`. | | ||
| | webInstrumentationConfig | Not currently supported by the shim, or Azure Monitor OpenTelemetry. | | ||
| | enableAutoCollectPerformance | Not supported in the shim. | | ||
| | enableAutoCollectConsole | Not supported in the shim. | |
There was a problem hiding this comment.
I see, performance counters appear to be supported in the shim code, although not via the distro's API any longer.
There was a problem hiding this comment.
Yeah but this shim readme not distro
| | webInstrumentationConfig | Not currently supported by the shim, or Azure Monitor OpenTelemetry. | | ||
| | enableAutoCollectPerformance | Not supported in the shim. | | ||
| | enableAutoCollectConsole | Not supported in the shim. | | ||
| | enableAutoCollectExternalLoggers | Not supported in the shim. | |
There was a problem hiding this comment.
I see performance counters will be, and Console + Winston are not supported in OTel yet, so I'll keep those instrumentations here. But I believe we need to kill webInstrumentationConfig since configuring the browser SDK loader is no longer supported in the distro.
There was a problem hiding this comment.
Yeah I didn't say anything about webInstrumentationConfig, we should deprecate that one
There was a problem hiding this comment.
It's in the highlighted bit in the UI on Github on my side, sorry for the confusion!
| | enableAutoCollectPerformance | Not supported in the shim. | | ||
| | enableAutoCollectConsole | Not supported in the shim. | | ||
| | enableAutoCollectExternalLoggers | Not supported in the shim. | | ||
| | enableAutoCollectExceptions | Not supported in the shim. | |
There was a problem hiding this comment.
Do you have a task to follow up on this?, it would be good to check if we can support this
| winston: { enabled: true }, | ||
| bunyan: { enabled: true } | ||
| }, | ||
| otlpTraceExporterConfig: {}, |
There was a problem hiding this comment.
Why removing OTLP configurations here? is it being initialized somewhere else not sure I follow
There was a problem hiding this comment.
Ah, this was a reaction to the API change in the distro, looks like I can modify the internal config of the distro from within main.ts so I'll just transition what used to be configurations passed to the distro via its API to modifying the internal config.
| @@ -160,23 +149,13 @@ class Config implements IConfig { | |||
| if (this.aadTokenCredential) { | |||
| options.azureMonitorExporterOptions.credential = this.aadTokenCredential; | |||
| } | |||
| if (typeof (this.enableAutoCollectConsole) === "boolean") { | |||
| const setting: boolean = this.enableAutoCollectConsole; | |||
| options.logInstrumentationOptions = { | |||
There was a problem hiding this comment.
Console OTel instrumentation still doesn't exist, so we cannot remove this
| if (typeof (this.enableAutoCollectExternalLoggers) === "boolean") { | ||
| options.logInstrumentationOptions = { | ||
| ...options.logInstrumentationOptions, | ||
| winston: { enabled: this.enableAutoCollectExternalLoggers }, |
There was a problem hiding this comment.
Same here winston still not collecting logs in OpenTelemetry instrumentation
| @@ -245,47 +210,6 @@ class Config implements IConfig { | |||
| process.env["APPLICATION_INSIGHTS_NO_STANDARD_METRICS"] = "disable"; | |||
| } | |||
| } | |||
| // NATIVE METRICS | |||
| if (typeof (this.enableAutoCollectExtendedMetrics) === "boolean") { | |||
There was a problem hiding this comment.
If you are removing native metrics please remove any related code, including all types, only updating the config is not enough
Cleaning the shim of options no longer supported by the distro API after being updated.
Including
Resolves bug where if enableAutoCollectDependencies/enableAutoCollectRequests was set to false, all other instrumentation options would be false.
Cleans up the unused
configHelperclass.