feat: support manual reporting of metrics in toolkit #205
Conversation
Signed-off-by: shyunny <shyunny@outlook.com>
Signed-off-by: shyunny <shyunny@outlook.com>
Signed-off-by: shyunny <shyunny@outlook.com>
Signed-off-by: shyunny <shyunny@outlook.com>
Signed-off-by: shyunny <shyunny@outlook.com>
Signed-off-by: shyunny <shyunny@outlook.com>
toolkit/metric/api.go
Outdated
| @@ -18,16 +18,21 @@ | |||
| package metric | |||
|
|
|||
| // NewCounter creates a new counter metrics. | |||
| func NewCounter(name string, opt ...MeterOpt) *CounterRef { | |||
| func NewCounter(name string, opt ...interface{}) *CounterRef { | |||
There was a problem hiding this comment.
Why we need change the opt as interface? If it necessary, please add comment in the method to describe what kind of data we need.
There was a problem hiding this comment.
Using the interface{} type will allow us to handle it better in the interceptor, at least we don't need to do heavy type conversion tasks, which will bring a lot of unnecessary work.
I will strive to write detailed comments to explain to users why and how we should use it.
There was a problem hiding this comment.
interface{} seems a very general type. For a counter, the parameters should be very certain. And building a counter is not a high-frequency operation, I can't see why change this.
There was a problem hiding this comment.
But it's not easy understanding in the user, right?
Can we add an opt structure, and using the enhance instance as the option configuration. Then, user can know which kind of opts they can fill the arguments.
There was a problem hiding this comment.
But it's not easy understanding in the user, right?
Not only this, more about. How to prevent the wrong implementation in the method? A certain type is a good thing, we should not expose an interface{} but no other condition, unless this is the use case.
| type NewHistogramInterceptor struct{} | ||
|
|
||
| func (h *NewHistogramInterceptor) BeforeInvoke(_ operator.Invocation) error { | ||
| return nil |
There was a problem hiding this comment.
Please using the defined result when interceptor all metrics constructor. It's better to control all object in the same side, if we change the toolkit API or agent, it could have unknown dependency error.
There was a problem hiding this comment.
I don't quite understand this, are you saying that we shouldn't use anonymous function parameters or do you mean something else?
There was a problem hiding this comment.
I mean is it possible we can construct the metrics by the plugin?
There was a problem hiding this comment.
This approach may be possible, but we need to consider the following issues:
- Parameter parsing is necessary in the interceptor. Even if we build it in the plugin, we still need to parse the parameters
- Using (1) to build a metric instance, if it is implemented in the plugin, will it introduce unnecessary redundant code?
- In fact, we don’t do anything complicated in the interceptor, as I said above: parse parameters -> build objects. When the dependent
NewXXXchanges, compatibility must be considered
So I think we may not need to change this
There was a problem hiding this comment.
Why it's related to the parameter? I just don't want to use the existing instance created from the toolkit API.
The basic process is: to build and define the result in the BeforeInvoke, then the toolkit implementation will not be invoked. The plugin controls all (toolkit)metrics instances, if we want to extend the metrics it's easy to adapt.
|
Do we have a complete doc? @ShyunnY Would you like to write a blog to introduce the API usage through a blog? |
|
yeah, I would write docs and blogs about the latest new features in |
In the current PR, I implemented the API defined in the
toolkit/metricpackage to support manual construction of metrics to record metric information.