feat: implement query_metrics

[cdoern]

What does this PR do?

query_metrics currently has no implementation, meaning once a metric is emitted there is no way in llama stack to query it from the store.

implement query_metrics for the meta_reference provider which follows a similar style to query_traces, using the trace_store to format an SQL query and execute it

in this case the parameters for the query are metric.METRIC_NAME, start_time, and end_time and any other matchers if they are provided.

this required client side changes since the client had no query_metrics or any associated resources, so any tests here will fail but I will provide manual execution logs for the new tests I am adding

order the metrics by timestamp.

Additionally add unit to the MetricDataPoint class since this adds much more context to the metric being queried.

depends on llamastack/llama-stack-client-python#260

Test Plan

import time
import uuid


def create_http_client():
    from llama_stack_client import LlamaStackClient

    return LlamaStackClient(base_url="http://localhost:8321")


client = create_http_client()

response = client.telemetry.query_metrics(metric_name="total_tokens", start_time=0)
print(response)

╰─ python3.12 ~/telemetry.py
INFO:httpx:HTTP Request: POST http://localhost:8322/v1/telemetry/metrics/total_tokens "HTTP/1.1 200 OK"
[TelemetryQueryMetricsResponse(data=None, metric='total_tokens', labels=[], values=[{'timestamp': 1753999514, 'value': 34.0, 'unit': 'tokens'}, {'timestamp': 1753999816, 'value': 34.0, 'unit': 'tokens'}, {'timestamp': 1753999881, 'value': 34.0, 'unit': 'tokens'}, {'timestamp': 1753999956, 'value': 34.0, 'unit': 'tokens'}, {'timestamp': 1754000200, 'value': 34.0, 'unit': 'tokens'}, {'timestamp': 1754000419, 'value': 36.0, 'unit': 'tokens'}, {'timestamp': 1754000714, 'value': 36.0, 'unit': 'tokens'}, {'timestamp': 1754000876, 'value': 36.0, 'unit': 'tokens'}, {'timestamp': 1754000908, 'value': 34.0, 'unit': 'tokens'}, {'timestamp': 1754001309, 'value': 584.0, 'unit': 'tokens'}, {'timestamp': 1754001311, 'value': 138.0, 'unit': 'tokens'}, {'timestamp': 1754001316, 'value': 349.0, 'unit': 'tokens'}, {'timestamp': 1754001318, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001320, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001341, 'value': 923.0, 'unit': 'tokens'}, {'timestamp': 1754001350, 'value': 354.0, 'unit': 'tokens'}, {'timestamp': 1754001462, 'value': 417.0, 'unit': 'tokens'}, {'timestamp': 1754001464, 'value': 158.0, 'unit': 'tokens'}, {'timestamp': 1754001475, 'value': 697.0, 'unit': 'tokens'}, {'timestamp': 1754001477, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001479, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001489, 'value': 298.0, 'unit': 'tokens'}, {'timestamp': 1754001541, 'value': 615.0, 'unit': 'tokens'}, {'timestamp': 1754001543, 'value': 119.0, 'unit': 'tokens'}, {'timestamp': 1754001548, 'value': 310.0, 'unit': 'tokens'}, {'timestamp': 1754001549, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001551, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001568, 'value': 714.0, 'unit': 'tokens'}, {'timestamp': 1754001800, 'value': 437.0, 'unit': 'tokens'}, {'timestamp': 1754001802, 'value': 200.0, 'unit': 'tokens'}, {'timestamp': 1754001806, 'value': 262.0, 'unit': 'tokens'}, {'timestamp': 1754001808, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001810, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001816, 'value': 82.0, 'unit': 'tokens'}, {'timestamp': 1754001923, 'value': 61.0, 'unit': 'tokens'}, {'timestamp': 1754001929, 'value': 391.0, 'unit': 'tokens'}, {'timestamp': 1754001939, 'value': 598.0, 'unit': 'tokens'}, {'timestamp': 1754001941, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001942, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754001952, 'value': 252.0, 'unit': 'tokens'}, {'timestamp': 1754002053, 'value': 251.0, 'unit': 'tokens'}, {'timestamp': 1754002059, 'value': 375.0, 'unit': 'tokens'}, {'timestamp': 1754002062, 'value': 244.0, 'unit': 'tokens'}, {'timestamp': 1754002064, 'value': 111.0, 'unit': 'tokens'}, {'timestamp': 1754002065, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754002083, 'value': 719.0, 'unit': 'tokens'}, {'timestamp': 1754002302, 'value': 279.0, 'unit': 'tokens'}, {'timestamp': 1754002306, 'value': 218.0, 'unit': 'tokens'}, {'timestamp': 1754002308, 'value': 198.0, 'unit': 'tokens'}, {'timestamp': 1754002309, 'value': 69.0, 'unit': 'tokens'}, {'timestamp': 1754002311, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754002324, 'value': 481.0, 'unit': 'tokens'}, {'timestamp': 1754003161, 'value': 579.0, 'unit': 'tokens'}, {'timestamp': 1754003161, 'value': 69.0, 'unit': 'tokens'}, {'timestamp': 1754003169, 'value': 499.0, 'unit': 'tokens'}, {'timestamp': 1754003171, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754003173, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754003185, 'value': 422.0, 'unit': 'tokens'}, {'timestamp': 1754003448, 'value': 579.0, 'unit': 'tokens'}, {'timestamp': 1754003453, 'value': 422.0, 'unit': 'tokens'}, {'timestamp': 1754003589, 'value': 579.0, 'unit': 'tokens'}, {'timestamp': 1754003609, 'value': 279.0, 'unit': 'tokens'}, {'timestamp': 1754003614, 'value': 481.0, 'unit': 'tokens'}, {'timestamp': 1754003706, 'value': 303.0, 'unit': 'tokens'}, {'timestamp': 1754003706, 'value': 51.0, 'unit': 'tokens'}, {'timestamp': 1754003713, 'value': 426.0, 'unit': 'tokens'}, {'timestamp': 1754003714, 'value': 70.0, 'unit': 'tokens'}, {'timestamp': 1754003715, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754003724, 'value': 225.0, 'unit': 'tokens'}, {'timestamp': 1754004226, 'value': 516.0, 'unit': 'tokens'}, {'timestamp': 1754004228, 'value': 127.0, 'unit': 'tokens'}, {'timestamp': 1754004232, 'value': 281.0, 'unit': 'tokens'}, {'timestamp': 1754004234, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754004236, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754004244, 'value': 206.0, 'unit': 'tokens'}, {'timestamp': 1754004683, 'value': 338.0, 'unit': 'tokens'}, {'timestamp': 1754004690, 'value': 481.0, 'unit': 'tokens'}, {'timestamp': 1754004692, 'value': 124.0, 'unit': 'tokens'}, {'timestamp': 1754004692, 'value': 65.0, 'unit': 'tokens'}, {'timestamp': 1754004694, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754004703, 'value': 211.0, 'unit': 'tokens'}, {'timestamp': 1754004743, 'value': 338.0, 'unit': 'tokens'}, {'timestamp': 1754004749, 'value': 211.0, 'unit': 'tokens'}, {'timestamp': 1754005566, 'value': 481.0, 'unit': 'tokens'}, {'timestamp': 1754006101, 'value': 159.0, 'unit': 'tokens'}, {'timestamp': 1754006105, 'value': 272.0, 'unit': 'tokens'}, {'timestamp': 1754006109, 'value': 308.0, 'unit': 'tokens'}, {'timestamp': 1754006110, 'value': 61.0, 'unit': 'tokens'}, {'timestamp': 1754006112, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754006130, 'value': 705.0, 'unit': 'tokens'}, {'timestamp': 1754051825, 'value': 454.0, 'unit': 'tokens'}, {'timestamp': 1754051827, 'value': 152.0, 'unit': 'tokens'}, {'timestamp': 1754051834, 'value': 481.0, 'unit': 'tokens'}, {'timestamp': 1754051835, 'value': 55.0, 'unit': 'tokens'}, {'timestamp': 1754051837, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754051845, 'value': 102.0, 'unit': 'tokens'}, {'timestamp': 1754099929, 'value': 36.0, 'unit': 'tokens'}, {'timestamp': 1754510050, 'value': 598.0, 'unit': 'tokens'}, {'timestamp': 1754510052, 'value': 160.0, 'unit': 'tokens'}, {'timestamp': 1754510064, 'value': 725.0, 'unit': 'tokens'}, {'timestamp': 1754510065, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754510067, 'value': 133.0, 'unit': 'tokens'}, {'timestamp': 1754510083, 'value': 535.0, 'unit': 'tokens'}, {'timestamp': 1754596582, 'value': 36.0, 'unit': 'tokens'}])]

adding tests for each currently documented metric in llama stack using this new function. attached is also some manual testing

integrations tests passing locally with replay mode and the linked client changes:

[franciscojavierarceo]

This is great. It's probably worth throwing a unit test showing the intended aggregation behavior of the telemetry.

I haven't spent much time on this part of the stack yet but I would love to add a page in the UI for telemetry with some basic info.

[cdoern]

@franciscojavierarceo great idea! I will add some unit tests. I am also planning on adding a bunch of integration tests for telemetry using query_metrics as a part of this PR

[ashwinb]

why is this conditional? can the meta-reference provider have no trace-store? in general, this feels odd. at the very least, you should error when there is no trace store and someone is querying for metrics instead of silently returning nothing.

[cdoern]

true, I think this was to satisfy mypy. I will double check if trace_store can be nil, and if it can't just remove this but if it can, raise an error.

[ashwinb]

thank you for writing a meaningful and true integration test!

[cdoern]

I think this is set, please let me know if any of my SQL queries look a bit off, tried my best to make them make sense cc @ehhuang

not sure how we want to deal with merging the client side changes and the test failures here due to those.

[ashwinb]

not sure how we want to deal with merging the client side changes and the test failures here due to those.

Can you explain what the missing client-side change is which results in the failure? I haven't dug into the failures.

[cdoern]

sure @ashwinb

all of the tests/integration/telemetry/test_telemetry_metrics.py failures are expected, I just fixed the other ones which were just due to some model changes I made locally when recording.

Those telemetry expected failures pass locally for me (see the image in PR description) and will pass here once llamastack/llama-stack-client-python#260 merges

[cdoern]

in order for the integration tests to pass, llamastack/llama-stack-client-python#19 needs to merge first and be released. But the tests pass locally for me.

The client changes are harmless to merge first since they add types and resources for an existing Api route

[ashwinb]

in order for the integration tests to pass, llamastack/llama-stack-client-python#19 needs to merge first and be released. But the tests pass locally for me.

How did you make those changes @cdoern ? We need to make them via Stainless and I believe you don't have access. We haven't yet made Stainless fully self-serve although we have been making gradual progress.

[cdoern]

in order for the integration tests to pass, llamastack/llama-stack-client-python#19 needs to merge first and be released. But the tests pass locally for me.

How did you make those changes @cdoern ? We need to make them via Stainless and I believe you don't have access. We haven't yet made Stainless fully self-serve although we have been making gradual progress.

I did them manually! I have done quite a few of these client-server shuffles so I kind of knew my way around the codebase :)

also these types were similar enough to query_spans on the client side so it wasn't too bad

[cdoern]

@ehhuang check me on this, but I noticed in other tests that use the openai_client helper, we need to skip when using library client?

Also, technically shouldn't llama_stack_client.chat.completions use the openai route? in our integration suite only the openai_client routes through /v1/openai/v1

[ashwinb]

@cdoern that is correct. if you use the compat_client fixture it automatically does this skipping.

[ashwinb]

@cdoern can you use the compat_client fixture everywhere please and remove the hasattr() check below?

[cdoern]

@ashwinb I was not able to use the compat_client for the following reasons:

1. when using compat_client I do not have access to .telemetry.query_metrics as that is not OpenAI compatible
2. when using compat_client the setup_telemetry_metrics_data would error out half of the time saying AttributeError: 'dict' object has no attribute 'prompt_tokens'. This is because openai completions format their .usage in a different format than our API.

What I was able to do is get rid of manual testing for .inference.chat_completion which is deprecated so all this does now is use the openai_client to create completions and the client_with_models to query .telemetry. Let me know if this is ok for now. I regenerated all of the json

[ashwinb]

@cdoern this is okay for now. I think once we have the client-stubs generated, we can see how we should think about it. re: usage data being differently formatted, we should probably adopt the same format right?

[cdoern]

hey @ehhuang @ashwinb wondering if I could get some more eyes on this, thanks!

[ashwinb]

I did them manually! I have done quite a few of these client-server shuffles so I kind of knew my way around the codebase :)

yeah we cannot do them manually. They need to be done via Stainless only otherwise the next stainless update will wipe them. Can you disable all the tests which depend on client types? Then I will regenerate the next client-SDK update based on the new openapi spec and once that lands we can re-enable the tests.

(on the side, I am working on making stainless automatic.)

[ashwinb]

@cdoern can you use the compat_client fixture everywhere please and remove the hasattr() check below?