ref(node): Infer orchestrion integration names#21834
Conversation
mydea
requested review from
JPeer264 and
andreiborza
and removed request for
a team
|
|
||
| // Same name as the OTel integration by design — when enabled, the OTel | ||
| // 'LruMemoizer' integration is omitted from the default set. | ||
| const INTEGRATION_NAME = 'LruMemoizer'; |
There was a problem hiding this comment.
q: is as const mandatory- there quite some without theas const`. If this is mandatory (or at least preferred) I could add them for the missing ones
There was a problem hiding this comment.
it is not mandatory, just nice to have and can make it easier when passing integrations around (as the names can then be inferred instead of just being any string). no harm if it remains non-constant in places :)
| integrations: [mysqlChannelIntegration(), lruMemoizerChannelIntegration()], | ||
| replacedOtelIntegrationNames: ['Mysql', 'LruMemoizer'], | ||
| setDiagnosticsChannelInjectionLoader((): DiagnosticsChannelInjection => { | ||
| const integrations = [mysqlChannelIntegration(), lruMemoizerChannelIntegration()] as const; |
There was a problem hiding this comment.
q: Would it make sense to move the integrations somewhere at the top of the file? Then it is easier to just add them at the top of the file
There was a problem hiding this comment.
I think this would change things because then it would be a side effect, it has to be in some function I suppose. we could extract it into a sub-function at the top I guess, can look at this in a follow up too :)
2 participants
Instead of requiring us to keep this as a string list, we can infer this from the integrations that are defined.
Also small cleanup of integration names to make infernal possible (not really used right now but may be helpful to debug things in the future...)