-
Notifications
You must be signed in to change notification settings - Fork 12
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
Use options object for all configuration #19
Conversation
…try and option to expose performance metrics
README.md
Outdated
|Name |Description |Default | | ||
|--------------------------|------------------------------------------------------------------------|--------------| | ||
|`circuits` |A list or individual circuit breaker to create metrics for |No circuits | | ||
|`registry` |An existing registry to use for prometheus metrics |A new registry| |
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.
i think the default here is undefined, or something like that
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.
I have added some more information about this option
README.md
Outdated
``` | ||
|
||
## Options | ||
The second argument of the `PrometheusMetrics` constructor is an options object. |
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.
no longer the second argument
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.
Thanks I have corrected this
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.
I think this is good to go, but can you split this into 2 PR's
the first is the breaking change making the argument the options object
And the second PR is making the expose performance metrics optional
How does this look now? Should I close #18 and I can raise another MR once this is in master? |
index.js
Outdated
registers: [this._registry], | ||
labelNames: ['name', 'event'] | ||
}); | ||
if (this.exposePerformanceMetrics()) { |
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.
I think this should be in the next PR instead of this one
index.js
Outdated
} | ||
} | ||
|
||
exposePerformanceMetrics () { |
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.
same as the previous comment
index.js
Outdated
@@ -55,7 +59,8 @@ class PrometheusMetrics { | |||
this._counter.labels(circuit.name, eventName).inc(); | |||
}); | |||
|
|||
if (eventName === 'success' || eventName === 'failure') { | |||
if (this.exposePerformanceMetrics() && |
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.
same as the previous comment
@HarryEMartland thanks for keeping up with this :) |
No description provided.