-
Notifications
You must be signed in to change notification settings - Fork 306
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
add metric object type #837
Conversation
datadog/dogstatsd/metrics.py
Outdated
super(SetMetric, self).__init__(name, tags, rate) | ||
self.data = set() | ||
self.data.add(value) | ||
self.lock = Lock() |
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.
Similarly - maybe we should wait to add synchronization primitives until we actually have multithreaded code running. We can keep this PR focused to just introducing the metric classes and their associated properties.
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.
the serialization function needs values like namespace and container_id which is set in the base.py class. I think it would be more convenient to leave it as a function in base.py or move it to aggregator.py? Not sure.
This is what aggregator.py looks like so far
Maybe we can move the serialization function there since it should have access to the DogStatsd client (it does in the existing go code) and its properties? It seems more intuitive that aggregator.py would handle serialization of a metric.
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.
LGTM!
/merge |
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
/remove |
🚂 Devflow: |
/merge |
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
/merge |
🚂 MergeQueue: waiting for PR to be ready This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
What does this PR do?
This PR is for adding metric objects that will make aggregation easier by allowing us to have one Metric object instantiation for each unique metric context that we can add samples to which will make aggregation easier. Existing behavior already exists in the client side aggregation for GO
Description of the Change
Create metric object type (
MetricAgggregator
) that other metric objects (CountMetric
,GaugeMetric
, etc.) implement that will enforce the objects to implement functionsaggregate
and potentiallyflush_unsafe
. This should also allow us to group the different metric types together in one list to be sent when we flush the metrics.Possible Drawbacks
Some future metrics may not have the necessary fields to be of type
MetricAggregator
The existing code for flushing metrics does NOT use object oriented programming, as a result they have duplicate code for each metric type.
![image](https://private-user-images.githubusercontent.com/171708865/343920392-2ff6fa05-62f3-4655-8c1a-325648ea76ea.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MDcxNDAsIm5iZiI6MTczOTYwNjg0MCwicGF0aCI6Ii8xNzE3MDg4NjUvMzQzOTIwMzkyLTJmZjZmYTA1LTYyZjMtNDY1NS04YzFhLTMyNTY0OGVhNzZlYS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE1JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNVQwODA3MjBaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT02MGUyOTFhZDJlNzQwNDc0YjcyYjk1NjFkYmVmMjcwODIwMzA4MDE0OTRjZmY2ODQ1N2NhYmYyMTYwYjRlNDJmJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Hg86kA9tISfxj51uHlCDlCGY8T7dyKQqBB8ryHz4siA)
Maybe i'm missing the potential drawbacks to using object oriented programming (specifically inheritance)?
Update:
![image](https://private-user-images.githubusercontent.com/171708865/344248725-a9415af9-8b00-427b-ad6e-bcaf575acb90.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MDcxNDAsIm5iZiI6MTczOTYwNjg0MCwicGF0aCI6Ii8xNzE3MDg4NjUvMzQ0MjQ4NzI1LWE5NDE1YWY5LThiMDAtNDI3Yi1hZDZlLWJjYWY1NzVhY2I5MC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE1JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNVQwODA3MjBaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT01ZjA3YTM1YTAzNjQwN2Q0NmJiOTgyNTA4ODA4MWIzMGRkZThhYjk2YjA3ZDc5NGI0MDM3MmExZGEzZDllOTFhJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.-8b8WxXGX18zSN_P5EqE0VoOD2mdDnnLCh79mNj9pmY)
Verification Process
Unit tests that verify the new metric objects match the existing behavior in the client side aggregator for Go.
Additional Notes
The python dogstatsd API will need to be refactored in the future to use these metric objects.
Release Notes
N/A, no new behavior until the aggregator class is implemented
Review checklist (to be filled by reviewers)
changelog/
label attached. If applicable it should have thebackward-incompatible
label attached.do-not-merge/
label attached.kind/
andseverity/
labels attached at least.