-
Notifications
You must be signed in to change notification settings - Fork 201
feat(langfuse): embedder, retriever and generator as obs. type #2497
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
feat(langfuse): embedder, retriever and generator as obs. type #2497
Conversation
|
hey @vblagoje I'm assigning you since you have more context |
vblagoje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just make this one minor change if you agree. Lowers cognitive load quite a lot.
| elif context.operation_name == "haystack.agent.run": | ||
| return LangfuseSpan(self.tracer.start_as_current_observation(name=context.name, as_type="agent")) | ||
| elif context.component_type in _ALL_SUPPORTED_GENERATORS: | ||
| elif context.component_type and context.component_type.endswith("Retriever"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I started this smell here, so let's please update it to something that's easier to read:
span_type = "span"
if context.component_type == "ToolInvoker":
span_type = "tool"
elif context.operation_name == "haystack.agent.run":
span_type = "agent"
elif context.component_type and context.component_type.endswith("Retriever"):
span_type = "retriever"
elif context.component_type and context.component_type.endswith("Embedder"):
span_type = "embedding"
elif context.component_type and context.component_type.endswith("Generator"):
span_type = "generation"
return LangfuseSpan(self.tracer.start_as_current_observation(name=context.name, as_type=span_type))
LMK wdyt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! Much better now.
vblagoje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok looks gtg, thanks for this contrib @Hansehart, keep them coming
Related Issues
Remove hardcoded (chat) generator type checks from Langfuse tracer #2050
feat(langfuse): add cost and usage support for more generators and generally for embedders #2472
feat(langfuse): enhance trace capabilities by default with the types that langfuse offers #2473
Proposed Changes:
Adopting https://langfuse.com/docs/observability/features/observation-types for better observability. Langfuse types has been adopted where it makes sense. Beside better visual interaction, usage and cost tracking is for the appropriate types improved
How did you test it?
unit
Notes for the reviewer
Added embedder, generator and retriever. Also new logic for checking for generator type.
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.