#Compat for internal go code
1 messages · Page 1 of 1 (latest)
So the issue to me seems to be that because we changed that function signature, anyone who calls into the internal/telemetry package and calls .Close is going to hit this? The Bass SDK was affected because it did this, but also we'd have this issue if any other SDK or module called this too.
ATM, we're really only doing module compat for the generated code, but the internal/ packages like querybuilder and telemetry should also have compat I guess
which is maybe tricky. the way we did compat for those is through templating/etc, but those packages above are just copied directly
one option as a short-term solution could be to just not break those modules - e.g. we could have added CloseContext or similar, and then deprecated Close - like good go libraries, but this feels fiddly to do
another option would be to fully move telemetry and querybuilder out to a separate repo, version them entirely separately, which i know has been a potential option before?
or we introduce magical templating into the generation of those internal packages 😢
mayyybe leaning towards this one, will bring up in core-dev architecture sync today (cc @heavy adder @marsh silo)
I didn't do this originally because passing the ctx is the only correct way to call it now that we install the tracer/logger providers into the ctx for the per-client telemetry DBs in the engine. But you're right, I think we could keep the original Close() without breaking the other use cases, and mark it deprecated, if we just bring back the package-level vars. That begs the question of when we remove the deprecated bit though, bringing us back to square 1 as long as we're in the (noble) mindset of "don't break modules ever."
Actually, on second look this doesn't have much to do with per-client DBs in the engine; that gets handled a separate way, not via telemetry.Close. I think this was for consistency/consolidation more than anything else, since I had to move away from the package-level vars - which was triggered by per-client DBs, but didn't necessarily have to bubble up in this way.
Worth a think - it would align with each packages' existing interest of maintaining a tiny set of dependencies. As long as we're able to precisely pin the version expected by the engine's codegen, this seems fine. Only hurdle, I think, is making the separate repos, since I know we've wanted to avoid sprawl in the past. (I would also want one for testctx eventually btw so Go folks have a nice OTel option for testing)