-
Notifications
You must be signed in to change notification settings - Fork 51
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: add basic opentelemetry tracing support #75
base: main
Are you sure you want to change the base?
Conversation
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.
Just some drive-by comments requested by @codefromthecrypt, nothing should be considered authoritative or even beyond nits
self.exchange.add(message) | ||
self.reply() # Process the user message. | ||
except KeyboardInterrupt: | ||
span.set_status(OtelStatus(OtelStatusCode.ERROR, "KeyboardInterrupt")) |
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.
This is an interesting case. I don't know if an explicit interrupt would actually be an error for the user, it's expected to fail - a browser comparison would be closing a tab during a request, it would be common for the request to still complete on the server side and be green. Anyways, it's probably correct to set it as an error though not fully confident
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.
added a comment as I don't know yet, either, but maybe after some practice someone will.
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.
hey idea.. maybe we set it to ok, but leave the interrupt?
Signed-off-by: Adrian Cole <[email protected]>
excellent feedback @anuraaga, thanks for dropping by! |
Signed-off-by: Adrian Cole <[email protected]>
This adds basic setup for tracing and some tests as a toe-hold. Later pull requests here can enhance the traces with more data. This would be even better if LLM spans were added as discussed here square/exchange#33
Meanwhile, this is what a trace looks like running against ollama using otel-tui as the tracing backend
If any maintainers have any glitch fixes they also have my permission to push directly to this branch.