-
-
Notifications
You must be signed in to change notification settings - Fork 494
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 Metrics API and aggregator #2247
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2247 +/- ##
==========================================
+ Coverage 97.41% 97.54% +0.12%
==========================================
Files 102 110 +8
Lines 3828 4026 +198
==========================================
+ Hits 3729 3927 +198
Misses 99 99
|
11f2f29
to
ac951a0
Compare
ac951a0
to
ee59473
Compare
e59667e
to
675292b
Compare
b635709
to
9569bf9
Compare
31f2de9
to
034b84d
Compare
034b84d
to
c1217cd
Compare
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.
Are the units defined somewhere? Or do we rely on people just following docs for the supported default units?
will add unit lists in separate PR |
e051335
to
8f9dabb
Compare
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.
Some comments.
I had not yet time to look at the tests.
|
||
context 'with pending buckets' do | ||
before do | ||
allow(Time).to receive(:now).and_return(fake_time) |
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.
Maybe we should start using timecop for these tests.
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.
for now it's simple enough, if you really want I can refactor all the time related tests separately.
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.
From my limited understanding of Ruby this looks OK. Could not find anything that was screaming at me. (It also looks quite similar to the Python implementation.)
@st0012 im gonna start merging these because we want to ship this week. If you have things you want changed, feel free to leave reviews anyway, I can change them separately. |
Sentry::Metrics
module with 4 apis that map to the new 4Sentry::Metrics::Metric
classesincrement
- simple counterdistribution
- array of observationsgauge
- statistics (last/min/max/sum/count)set
- unique valuesSentry::Metrics::Aggregator
that starts a thread that flushes pending metric buckets in 5 second intervalsflush_shift
once per startup to create jitteringstatsd
type envelope that is not json so made a small change to theEnvelope::Item
Reference spec - https://develop.sentry.dev/sdk/metrics/
part of #2246