-
Notifications
You must be signed in to change notification settings - Fork 268
Add service to stream custom otel traces to otel-collector #262
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
Conversation
|
|
||
|
|
||
| with open(envoy_log_path, "r") as f: | ||
| while True: |
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.
I think best to check if the log file exists to begin with, if not that means tracing was off - then no need to run this for loop.
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.
it must exist - if it doesnt we should fail/crash
I also make sure to create this file in Dockerfile see here -https://github.com/katanemo/arch/pull/262/files#diff-8e3a6a6de1bde2a67bc1791e073f4724332070eac7b751f83122f8929eb87f1fR9
| attributes: vec![], | ||
| }); | ||
|
|
||
| trace_data.add_span(Span { |
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.
I think this should be an even in the total_time span. We shouldn't represent TTFT as a separate span because there is technically no new service work. https://opentelemetry.io/docs/concepts/signals/traces/#span-events
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.
using event now
After this change we can generate custom spans showing total time taken by upstream llm