feat(telemetry): updated semantic conventions, added timeToFirstByteMs into spans and metrics #997
Conversation
poshinchen
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
poshinchen
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
poshinchen
changed the title
poshinchen
had a problem deploying
to
auto-approve
GitHub Actions
Failure
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
poshinchen
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
poshinchen
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
|
|
||
| latencyMs: int | ||
| latencyMs: Required[int] | ||
| timeToFirstByteMs: int |
There was a problem hiding this comment.
Can you edit to be as precise as possible in the description what timeToFirstByteMs actually means here It looks like the definition is actually ("contentBlockDelta" in chunk or "contentBlockStart" in chunk)
There was a problem hiding this comment.
Good callout. Should the checking logic look for the first chunk returned from the model provider, or for the first "contentBlockDelta"/"contentBlockStrat" from the model provider?
| chunks = model.stream(messages, tool_specs if tool_specs else None, system_prompt) | ||
|
|
||
| async for event in process_stream(chunks): | ||
| async for event in process_stream(chunks, start_time): |
There was a problem hiding this comment.
ok so we need start_time passed in rather than just starting it in process_stream because we are actually calling mode.stream in stream_messages?
I guess this comes down to clearly knowing what customers want, and is related to the comment about the Metrics type. Meaning should this go before BeforeModelCallEvent or not
There was a problem hiding this comment.
ok so we need start_time passed in rather than just starting it in process_stream because we are actually calling mode.stream in stream_messages?
Yes
I guess this comes down to clearly knowing what customers want, and is related to the comment about the Metrics type. Meaning should this go before BeforeModelCallEvent or not
correct, the current implementation is to count the latency of sending / getting the first chunk from the model.
|
|
||
| latencyMs: int | ||
| latencyMs: Required[int] | ||
| timeToFirstByteMs: int |
There was a problem hiding this comment.
Good callout. Should the checking logic look for the first chunk returned from the model provider, or for the first "contentBlockDelta"/"contentBlockStrat" from the model provider?
Description
timeToFirstByteMsinto the spans and metricsRelated Issues
#986
Documentation PR
N/A
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.