[Shim] Fix JSON Config #1203
[Shim] Fix JSON Config #1203
Conversation
JacksonWeber
commented
Sep 6, 2023
•
JacksonWeber
commented
- Add remaining JSON config values.
- Fix JSON config file path and stop us from overwriting defined env vars with undefined values from the JSON file.
JacksonWeber
changed the title
| isInitialized = true; | ||
| const individualOptOuts: string = ShimJsonConfig.getInstance().noPatchModules; |
There was a problem hiding this comment.
Why are you adding the check here? you should be checking for config options in the subscriber code not here in publishers side
There was a problem hiding this comment.
Rationale was that we used to disable diagnostic channels on the publisher side, however I agree it makes sense to disable them at a subscriber level. Will update that.
There was a problem hiding this comment.
Refactored this to use the publisher's enable functions (specifically not calling them if noDiagnosticChannel is set to true) in the same way as noPatchModules.
There was a problem hiding this comment.
Not seeing those changes here, is there a commit missing?
There was a problem hiding this comment.
Changed such that we still do this check on the publisher side (line 26 is where we do the check for noDiagnosticChannel). It takes advantage of the same enable() methods each of the subscribers has. If we don't want to do this check for noDiagnosticChannel here we could move the logic for both noDiagnosticChannel and noPatchModules to each individual subscriber, but after looking into moving the code to the subscribers it seems like it'd make this logic more complicated than necessary.
| @@ -4,14 +4,17 @@ | |||
| // Don't reference modules from these directly. Use only for types. | |||
| import * as DiagChannelPublishers from "diagnostic-channel-publishers"; | |||
| import { Logger } from "../../shared/logging"; | |||
| import { ShimJsonConfig } from "../../shim/jsonConfig"; | |||
There was a problem hiding this comment.
JSON config should never used outside of our Internal config class, value could be overridden using JS code so this ignores that
There was a problem hiding this comment.
noDiagnosticChannel and noPatchModules cannot be configured via defaultClient.config.noDiagnosticChannel or defaultClient.config.noPatchModules, only via env var or JSON config, so I don't believe this could be overridden before being read here. Were you referring to another way it could be overridden?
