-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: metrics schema version #12732
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
base: master
Are you sure you want to change the base?
feat: metrics schema version #12732
Conversation
… and LabeledCounter
5025e2a
to
8fdc9e8
Compare
Test Results - Alternative Providers987 tests 589 ✅ 30m 27s ⏱️ Results for commit f276526. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 22m 21s ⏱️ Results for commit f276526. ♻️ This comment has been updated with latest results. |
@@ -132,15 +151,25 @@ class LabeledCounter(Metric): | |||
""" | |||
|
|||
_type: str | |||
_schema_version: int |
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.
note: for now, I’m adding schema_version
only to the counter implementations. If it proves to be relevant for other metric types as well, we’ll move its definition up to the base Metric
class.
|
||
def __init__(self, namespace: str, name: str): | ||
def __init__(self, namespace: str, name: str, schema_version: int = 1): |
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.
note: default is set to 1 and automatically applied to all existing counters.
if schema_version is None: | ||
raise ValueError("Labeled metrics require an explicit schema version.") |
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.
thought: It shouldn’t ever happen given the default is set to 1,
question: should we remove the check, or simply keep it as an extra safeguard?
|
||
if not isinstance(schema_version, int): | ||
raise TypeError("Schema version must be an integer.") | ||
|
||
if schema_version <= 0: | ||
raise ValueError("Schema version must be greater than zero.") |
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.
note: naive approach. At the moment, we can't verify whether the schema_version
is being incremented sequentially. However, once approved, we’ll explicitly document how schema_version
should be used going forward.
Motivation
This PR introduces explicit schema versioning for metrics definitions.
The goal is to make telemetry changes safer and more maintainable:
For now:.
However, having the
schema_version
available in Tinybird helps with:status
of a request, with possible values like "success" and "error". Later, the counter is updated: a new label label_2 is introduced to hold the status instead:A more structured approach would require:
Changes