-
Notifications
You must be signed in to change notification settings - Fork 235
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
[307] Add support for histograms and distributions without unit conversion #314
Conversation
Signed-off-by: glightfoot <[email protected]>
Signed-off-by: glightfoot <[email protected]>
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.
This looks great! Thank you for fixing the benchmarks.
You are right, we should rename the event. In line with the client library naming I would like to call it an ObserverEvent
, does that sound right?
Signed-off-by: glightfoot <[email protected]>
ObserverEvent makes sense and is nice and consistent. I pushed up that change now. The only thing I can think of now is that there are plenty of stats calls that still reference timer event:
I think it makes sense to change those to "observer", but that would be another breaking change to metrics usage, so let me know what you'd like to see. |
The other thing I can see is there are still lots of references to Timer in the Mapper pkg. I can change them, but then that breaks the configs since things like |
Signed-off-by: glightfoot <[email protected]>
Ah, you ask all the good questions. I'm fine with "breaking" the metrics, a note in the changelog will help, which I will add anyway. I am less willing to break configurations, how hard would it be to change the primary option but maintain compatibility? (i.e. document |
Ok I'll change the metrics now. I haven't looked too much at the mapper config code, so I am not sure how simple of a change that will be |
Signed-off-by: glightfoot <[email protected]>
Signed-off-by: glightfoot <[email protected]>
Signed-off-by: glightfoot <[email protected]>
I added some custom unmarshalers to handle both keys. |
Signed-off-by: glightfoot <[email protected]>
Signed-off-by: glightfoot <[email protected]>
…rshalers Signed-off-by: glightfoot <[email protected]>
I built this branch and tested with the following: testing unit conversion and deprecated config value
testing new config overriding deprecated timer_type
Is there anything else necessary for this PR? |
This is awesome, thank you! |
Update the README to reflect #314. Add an entry to the changelog with extended compatibility notes. Signed-off-by: Matthias Rampke <[email protected]>
Changelog and documentation update for #314
This fixes #307 by moving millisecond-->second conversion to the buildEvent function for
ms
statTypes and also adds a case forh
andd
statTypes without the conversion. This effectively makes the TimerEvent value in seconds/unitless. Since this is no longer just limited to timers, should the TimerEvent be renamed to DistributionEvent or something similar?Test cases were also added for the three distribution types and the conversions.
Summary:
ms
units in milliseconds, converted to seconds for prometheush
no units, no conversiond
no units, no conversionThe benchmark tests were broken, so the metrics were added back to them to get the benchmark working. That commit can be removed if necessary.
Before:
After:
These results show no significant increase in execution time, as the difference is within the variation observed on repeated benchmarking.