Sandersaarond/mysql model metrics

grafana / ai-training-o11y

GNU Affero General Public License v3.0

4 stars 3 forks source link

Sandersaarond/mysql model metrics #98

Closed SandersAaronD closed 1 month ago

SandersAaronD commented 2 months ago

WIP

Meant to transition writing and reading model metrics from loki to mysql, currently has a writer implemented and the reader function is there but not attached to an endpoint.

TODO for this:

[x] make sure the reader function works, and serves an endpoint
[x] have the UI query that endpoint instead of loki
[x] switch the python client to write to this (format was changed slightly to allow for debouncing so that we don't end up with too many http connections open at once for frequent logging jobs)

Unclear if this can be done as part of this PR, but: I have a dangling concern about authentication, where it isn't clear to me if tenantID will always be populated correctly in production, since our local setup just defaults to 0, making this hard to test locally. Either we need to find a way to test this locally or we won't know about that until we deploy it.

SandersAaronD commented 1 month ago

I think this is ready for an actual review, I have these TODO that I'd like to do before merging:

1) Standardize references to TenantID or StackID, I think I will favor StackID and try to make it a uint64 in all cases. 2) Spin off separate issues for things that are definitely good ideas but that seem likely to bog down this already rather heavy PR. These are:

Centralize boilerplate for testing, especially around db mocking/setup
Add timestamp storage for each incoming datapoint

csmarchbanks commented 1 month ago

Standardize references to TenantID or StackID, I think I will favor StackID and try to make it a uint64 in all cases.

I would say to be more consistent with other Grafana services (Mimir, Loki, Tempo) we should have it be a string. The number just happens to be how we identify instances in Grafana Cloud.

Spinning off separate issues for followup seems like a good idea.

SandersAaronD commented 1 month ago

Standardize references to TenantID or StackID, I think I will favor StackID and try to make it a uint64 in all cases.

I would say to be more consistent with other Grafana services (Mimir, Loki, Tempo) we should have it be a string. The number just happens to be how we identify instances in Grafana Cloud.

Spinning off separate issues for followup seems like a good idea.

Separate issues are spun off.

I was actually going off of "StackID as an integer" because it's what gcom gives back but that appears to be the anomaly here. I'll set this to TenantID and a string, might take a little fiddling because code around types here is coupled to this choice.