ONYX-28233: Add telemetry #2854
Conversation
szh
force-pushed
the
feature-conjur-telemetry
branch
3 times, most recently
from
6f40765
to
dad3c0d
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
szh
force-pushed
the
feature-conjur-telemetry
branch
from
dad3c0d
to
4a0c97e
Compare
szh
force-pushed
the
feature-conjur-telemetry
branch
from
4a0c97e
to
0d66ecc
Compare
|
@szh, awesome work on this feature. My only real ask is to add some unit tests. It looks like coverage is largely handled via controller integration tests. Your code looks well written and it looks like it'll be simple to add unit tests. |
szh
force-pushed
the
feature-conjur-telemetry
branch
from
d6d0a40
to
ba946a8
Compare
szh
force-pushed
the
feature-conjur-telemetry
branch
2 times, most recently
from
32335c8
to
925891f
Compare
szh
force-pushed
the
feature-conjur-telemetry
branch
from
2be30c6
to
0c9079e
Compare
|
|
||
| module Monitoring | ||
| module Middleware | ||
| class PrometheusCollector |
There was a problem hiding this comment.
Monitoring::Middleware::PrometheusCollector assumes too much for instance variable '@initialized'
|
|
||
| def find_operation(method, path) | ||
| Monitoring::Metrics::OPERATIONS.each do |op| | ||
| if op[:method] == method && op[:pattern].match?(path) |
There was a problem hiding this comment.
Monitoring::Middleware::PrometheusCollector#find_operation is controlled by argument 'method'
| setup_subscriber(metric) | ||
| end | ||
|
|
||
| def setup_subscriber(metric) |
There was a problem hiding this comment.
Monitoring::Metrics#setup_subscriber doesn't depend on instance state (maybe move it to another class?)
| @@ -0,0 +1,60 @@ | |||
| module Monitoring | |||
| module Metrics | |||
There was a problem hiding this comment.
Monitoring::Metrics has no descriptive comment
|
|
||
| def create_histogram_metric(metric) | ||
| histogram = ::Prometheus::Client::Histogram.new( | ||
| metric.metric_name, |
There was a problem hiding this comment.
Monitoring::Metrics#create_histogram_metric refers to 'metric' more than self (maybe move it to another class?)
|
Hey @szh (@doodlesbykumbi), If we're keeping the commit history, I suggest we remove the following commits, at a minimum, as they represent a no-op once combined: Generally speaking, I would also expect linting fixes to be squashed back into the original commit, especially in cases where the PR introduces the files for the first time, as these commits on their own don't meaningfully contribute to the story of this PR: With those history modifications, I think this makes sense to include with the commit history. I think we must remove the two no-op commits to avoid those existing in master history. The others are a strong preference, but if it's too onerous I'm not strictly opposed to them. |
Remove the 'include Rack::Test::Methods' where it is not needed. Remove importing 'rack/test' since rack seems to be already loaded
jtuttle
force-pushed
the
feature-conjur-telemetry
branch
from
0c9079e
to
1fb0fad
Compare
|
@micahlee I just made the suggested commit updates if you could re-review when you have a chance. Thanks! |
|
|
||
| def create_gauge_metric(metric) | ||
| gauge = ::Prometheus::Client::Gauge.new( | ||
| metric.metric_name, |
There was a problem hiding this comment.
Monitoring::Metrics#create_gauge_metric refers to 'metric' more than self (maybe move it to another class?)
|
|
||
| def create_counter_metric(metric) | ||
| counter = ::Prometheus::Client::Counter.new( | ||
| metric.metric_name, |
There was a problem hiding this comment.
Monitoring::Metrics#create_counter_metric refers to 'metric' more than self (maybe move it to another class?)
| @@ -0,0 +1,29 @@ | |||
| module Monitoring | |||
| module Metrics | |||
| class PolicyRoleGauge | |||
There was a problem hiding this comment.
Monitoring::Metrics::PolicyRoleGauge has at least 6 instance variables
| @@ -0,0 +1,29 @@ | |||
| module Monitoring | |||
| module Metrics | |||
| class PolicyResourceGauge | |||
There was a problem hiding this comment.
Monitoring::Metrics::PolicyResourceGauge has at least 6 instance variables
| end | ||
| end | ||
|
|
||
| def get_authenticator_counts(authenticators) |
There was a problem hiding this comment.
Monitoring::Metrics::AuthenticatorGauge#get_authenticator_counts doesn't depend on instance state (maybe move it to another class?)
|
Code Climate has analyzed commit 1fb0fad and detected 50 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 99.6% (50% is the threshold). This pull request will bring the total coverage in the repository to 87.6% (-0.7% change). View more on Code Climate. |
Desired Outcome
Merge telemetry feature branch
The Telemetry feature is complete but it was done on a feature branch. We will need to merge this back to master before we can truly consider it complete.
As part of the review, we should primarily do some sanity checking to verify that Telemetry is not on by default and is essentially invisible until enabled.
Once the cyberark/conjur changes are merged we will can also merge the quick-start guide.
Implemented Changes
Describe how the desired outcome above has been achieved with this PR. In
particular, consider:
Connected Issue/Story
CyberArk internal issue ID: ONYX-28233
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
READMEs) were updated in this PRBehavior
Security