feat(observability): structured JSON logs and event fields #66
Conversation
tao12345666333
requested review from
Xunzhuo,
rootfs and
wangchen615
as code owners
👥 vLLM Semantic Team NotificationThe following members have been identified for the changed files in this PR and have been automatically assigned: 📁
|
tao12345666333
force-pushed
the
feat/observability-pricing-metrics
branch
from
e729a58 to
77aa1c2
Compare
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| ) | ||
|
|
||
| // ModelCostUSD tracks the total USD cost attributed to each model | ||
| ModelCostUSD = promauto.NewCounterVec( |
There was a problem hiding this comment.
should USD be a specific naming suffix for this metric?
There was a problem hiding this comment.
if we want to support more units a dimension in metric should be easier to extend.
There was a problem hiding this comment.
Your suggestion is good.
Maybe I can change the metric name to llm_model_cost_total and add a currency label. e.g. llm_model_cost_total{currency="USD"}
But a trade-off needs to be made here.
Pros:
- Easy to extend, allowing us to add CNY, EUR, etc, in the future as needed
- Clearer. A metric name for "cost," with currency as the dimension
Cons:
- Aggregations must be done per-currency (sum by (currency, model) …). It’s easy to accidentally sum different currencies if you forget to filter.
There was a problem hiding this comment.
For the cons, maybe we should have clear docs around metrics, it is a bit bad to add more metrics when units increase linearly
There was a problem hiding this comment.
adding a currency label is a good idea!
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I created an issue for tracking.
#75
tao12345666333
force-pushed
the
feat/observability-pricing-metrics
branch
2 times, most recently
from
a3aeeb2 to
9395d4c
Compare
|
|
||
| import ( | ||
| "encoding/json" | ||
| "log" |
There was a problem hiding this comment.
maybe this is time to pick a logging pkg, e.g. https://betterstack.com/community/guides/logging/best-golang-logging-libraries/
There was a problem hiding this comment.
I prefer zap, but I'll create an issue to see what others think.
|
@tao12345666333 lgtm. |
metrics: add llm_model_cost_usd_total and llm_routing_reason_codes_total docs: add pricing section and cost/routing metrics chore: normalize config/config.yaml line endings Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
tao12345666333
force-pushed
the
feat/observability-pricing-metrics
branch
from
0e6f4f7 to
be20700
Compare
|
@tao12345666333 thanks for contribution, please followup with the comments. |
4 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #49
Release Notes: Yes/No