-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Obs AI Assistant] Remove commented out langtrace initialization and … #193381
base: main
Are you sure you want to change the base?
Conversation
…make classes agnostic to used conventions.
non-binding comment: I had no idea the dep was already not in package.json. I like the idea of clarifying the code in use, and doing this prior to changing the semantics. I suspect even if we did want to use langtrace with the data produced here, we would need to change this data as they already have upstream to the otel semantics in 1.27.0. |
Ah just to clarify to reduce any confusion, there is a dep on langtrace, but it is only used for the attributes schema, not the SDK itself. And as this PR doesn't mean to change any attributes, I have left that for now, but as you mention we will likely want to move towards the OTel conventions. https://github.com/elastic/kibana/blob/main/package.json#L996 |
non-binding suggestion is to localize the constants and remove the dep? bc seems it has several deps of its own, so maybe can prove we aren't affected by those? |
Good idea, we actually were mostly just using it for schema validation (using the typescript type) with a couple of values for events. I inlined those and removed the dep, especially since we're pretty confident we'll need to update these to the latest spec that's both OTel and langtrace. |
great to see more lines gone than present in the last commit! |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Langtrace core maintainer here. Happy to prioritize anything needed to make things easier for you all. Feel free to open an issue here |
@karthikscale3 thanks for all the support as always. Luckily you are one of the few re-using the otel specs for core genai signals, so hopefully even custom code will work out against the langtrace service. we'll keep reaching out if any bumps happen in pursuit of that! |
…make classes agnostic to used conventions.
Summary
Note, this PR changes absolutely no behavior, it is purely to try to make the code clearer for future improvements.
As discussed offline, it should be fine to remove the commented out initialization of langtrace SDK (we already do not have a dependency in package.json) and we can flesh out our story for initializing OpenTelemetry instrumentation in this component going forward. Note that the features of the langtrace SDK are
Especially because of the second point, we can consider enabling the OTel SDK directly without the langtrace SDK. But to repeat, this PR doesn't make any changes to behavior such as enabling the SDK, we can do it later.
In addition to removing the commented out code, it also renames some entry points with langtrace naming to be a more generic genai, e.g.
LangTracer
->GenAITracer
. This can make clearer that the actual instrumentation code is not using langtrace code but is using the OpenTelemetry API. Conventions, including attributes likellm.model
are based on older langtrace versions, but langtrace itself has also moved towards converging on OpenTelemetry semconv, so going forward we will probably want to do the same./cc @codefromthecrypt @dgieselaar
Checklist
Just removing commented out code and pure renaming.
Risk Matrix
Just removing commented out code and pure renaming.
For maintainers