-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
process_collector: Add Platform-Specific Describe for processCollector #1625
Changes from 2 commits
1a1b860
b8300bc
0bfbc37
aecb101
91c8973
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |
|
||
type processCollector struct { | ||
collectFn func(chan<- Metric) | ||
describeFn func(chan<- *Desc) | ||
pidFn func() (int, error) | ||
reportErrors bool | ||
cpuTotal *Desc | ||
|
@@ -122,33 +123,35 @@ func NewProcessCollector(opts ProcessCollectorOpts) Collector { | |
// Set up process metric collection if supported by the runtime. | ||
if canCollectProcess() { | ||
c.collectFn = c.processCollect | ||
c.describeFn = c.describe | ||
} else { | ||
c.collectFn = func(ch chan<- Metric) { | ||
c.reportError(ch, nil, errors.New("process metrics not supported on this platform")) | ||
} | ||
c.collectFn = c.defaultCollectFn | ||
c.describeFn = c.defaultDescribeFn | ||
} | ||
|
||
return c | ||
} | ||
|
||
// Describe returns all descriptions of the collector. | ||
func (c *processCollector) Describe(ch chan<- *Desc) { | ||
ch <- c.cpuTotal | ||
ch <- c.openFDs | ||
ch <- c.maxFDs | ||
ch <- c.vsize | ||
ch <- c.maxVsize | ||
ch <- c.rss | ||
ch <- c.startTime | ||
ch <- c.inBytes | ||
ch <- c.outBytes | ||
func (c *processCollector) defaultCollectFn(ch chan<- Metric) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we name this and below something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed in 0bfbc37 |
||
c.reportError(ch, nil, errors.New("process metrics not supported on this platform")) | ||
} | ||
|
||
func (c *processCollector) defaultDescribeFn(ch chan<- *Desc) { | ||
if c.reportErrors { | ||
ch <- NewInvalidDesc(errors.New("process metrics not supported on this platform")) | ||
} | ||
} | ||
|
||
// Collect returns the current state of all metrics of the collector. | ||
func (c *processCollector) Collect(ch chan<- Metric) { | ||
c.collectFn(ch) | ||
} | ||
|
||
// Describe returns all descriptions of the collector. | ||
func (c *processCollector) Describe(ch chan<- *Desc) { | ||
c.describeFn(ch) | ||
} | ||
|
||
func (c *processCollector) reportError(ch chan<- Metric, desc *Desc, err error) { | ||
if !c.reportErrors { | ||
return | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,14 @@ func canCollectProcess() bool { | |
return false | ||
} | ||
|
||
// describe returns all descriptions of the collector for js. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! I'm not sure why we have files for js and wasip1 that explicitly say "false" in can collect function, but other potentially also non supported environments will go through _other file. Maybe we can find the source of the file/blame to see why it was added vs going through We can do in other PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this may caused by below function is not available in js and wasip1 PR, but I think you are still right, we can at least merge those 2 since they are exactly the same today. Just left them now as they are, let me know what do you think, I can open a follow PR.
|
||
// Ensure that this list of descriptors is kept in sync with the metrics collected | ||
// in the processCollect method. Any changes to the metrics in processCollect | ||
// (such as adding or removing metrics) should be reflected in this list of descriptors. | ||
func (c *processCollector) processCollect(ch chan<- Metric) { | ||
// noop on this platform | ||
return | ||
c.defaultCollect(ch) | ||
} | ||
|
||
func (c *processCollector) describe(ch chan<- *Desc) { | ||
c.defaultDescribe(ch) | ||
} |
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.
Is it really a customer visible change? It might be mostly our tech-debt removal, right?
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.
removed in 0bfbc37