fix(server-utils): Ensure all orchestrion instrumentation lazy registers - #22518
fix(server-utils): Ensure all orchestrion instrumentation lazy registers#22518isaacs wants to merge 3 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
73c683a to
d752238
Compare
|
Going to port this atop #22443 once that lands, since there are some conflicts. UPDATE: this is now done. Single commit can base cleanly on |
6d6a126 to
464dc8f
Compare
…on Node (#22518) Co-Authored-By: isaacs <i@izs.me> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
464dc8f to
ae93c4e
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…azy registers on Node (#22518)
…on Node (#22518) Co-Authored-By: isaacs <i@izs.me> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
38d60ec to
7edaad4
Compare
…azy registers on Node (#22518)
…on Node (#22518) Co-Authored-By: isaacs <i@izs.me> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…azy registers on Node (#22518)
549f754 to
3f5b4ba
Compare
cf84acc to
f7f9237
Compare
f7f9237 to
254eee8
Compare
254eee8 to
4208c97
Compare
4208c97 to
565927e
Compare
…lazy register on Node (#22518)
…ations lazy register on Node (#22518)
797ce4a to
2d83921
Compare
2d83921 to
8d88bb4
Compare
8d88bb4 to
5ca08a7
Compare
5ca08a7 to
206b43f
Compare
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
I see now why this PR is so big. Overall this is a great addition. Just got minor comments
| return; | ||
| } | ||
|
|
||
| DEBUG_BUILD && |
There was a problem hiding this comment.
m: Some of the debug logs were removed entirely, and others do still exist, such as the one in AWS or langgraph. Is this wanted to be like that? Would be actually nice if that could live entirely somehow in incokeOrchestrionInstrumentation
There was a problem hiding this comment.
Good catch, yes, some of these were removed, but a few remained for no particular reason, just overlooked or weren't converted so didn't get changed.
Moving it into invokeOrchestrionInstrumentation: agreed, that's a good improvement. The helper already knows the module names, and it knows more than what the old logs said, because it can state why it fired (or didn't), so that can actually be much more useful than just dumping the config name.
| }, | ||
| ] satisfies InstrumentationConfig[]; | ||
|
|
||
| export const awsSdkModuleNames = getModuleNames(awsSdkConfig); |
There was a problem hiding this comment.
q: Is there a guard somehow for integrations where we forget to add this?
Not sure if there could be a time where we forget it, because we would need it for the integration itself, just thinking out loud
There was a problem hiding this comment.
Forgetting xModuleNames would raise a type issue, so I don't think it's a problem. But the bigger issue is if we have an integration that forgets to route through invokeOrchestrionInstrumentation entirely, just doing the setupOnce style that predated this PR. (There are a few that do this, but only non-default integration cases, where we don't really need to worry, because presumably the user knows they're going to be using that module eventually, if they're explicitly instrumenting it.)
That would potentially get us back to a state where we're blowing up the node channel budget again. However, I think this is relatively unlikely. If we add a new default instrumentation, we (or our agents) are mostly likely going to copypasta the pattern that we already have established.
|
👋 @mydea — Please review this PR when you get a chance! |
Co-authored-by: Jan Peer Stöcklmair <jan.peer@sentry.io>
3 participants
This ports and refactors the intent of #22387, using the mechanisms landed on
developin #22094, rather than the mechanisms in #22386 which are similar in intent, but substantially different in implementation.The difference from #22387 is entirely in the plumbing underneath the helper. The way that "is my module injected?" and "tell me when it gets injected" are answered, both now use the machinery that already landed.
Beyond that, the actual registration, event emitting, double-wrap guard, and integration refactoring, should all look very familiar.