process_collector: Add Platform-Specific Describe for processCollector #1625
Conversation
ying-jeanne
force-pushed
the
split-describe-by-platform
branch
from
bc2ac02 to
acd48a9
Compare
Signed-off-by: Ying WANG <ying.wang@grafana.com>
ying-jeanne
force-pushed
the
split-describe-by-platform
branch
from
acd48a9 to
1a1b860
Compare
Signed-off-by: Ying WANG <ying.wang@grafana.com>
CHANGELOG.md
Outdated
| @@ -1,5 +1,7 @@ | |||
| ## Unreleased | |||
|
|
|||
| * [ENHANCEMENT] process_collector: Add Platform-Specific Describe for processCollector. #1625 | |||
There was a problem hiding this comment.
Is it really a customer visible change? It might be mostly our tech-debt removal, right?
There was a problem hiding this comment.
removed in 0bfbc37
prometheus/process_collector.go
Outdated
| ch <- c.startTime | ||
| ch <- c.inBytes | ||
| ch <- c.outBytes | ||
| func (c *processCollector) defaultCollectFn(ch chan<- Metric) { |
There was a problem hiding this comment.
Should we name this and below something like errorCollectFn and errorDescribeFn given what they do?
There was a problem hiding this comment.
renamed in 0bfbc37
| @@ -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.
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 _other file flow 🤔
We can do in other PR
There was a problem hiding this comment.
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.
func canCollectProcess() bool {
_, err := procfs.NewDefaultFS()
return err == nil
}
Signed-off-by: Ying WANG <ying.wang@grafana.com>
ying-jeanne
force-pushed
the
split-describe-by-platform
branch
from
de6aff8 to
0bfbc37
Compare
|
could you take another look at this PR @bwplotka @ArthurSens @kakkoyun? thank you. |
prometheus#1625) * process_collector: Add Platform-Specific Describe for processCollector Signed-off-by: Ying WANG <ying.wang@grafana.com> * add changelog entry Signed-off-by: Ying WANG <ying.wang@grafana.com> * Address comments Signed-off-by: Ying WANG <ying.wang@grafana.com> --------- Signed-off-by: Ying WANG <ying.wang@grafana.com> Signed-off-by: Eugene <eugene@amberpixels.io>
This PR addresses the issue described in #1591.
Given the clarity of the ticket, I proceeded directly with the fix rather than engaging in further discussion, but please let me know if there are any questions or concerns.
In this PR, I've refactored the Describe function to be platform-specific and added tests to ensure alignment between Describe and ProcessCollect for Linux and Windows. I've also included comments to help ensure future alignment between these functions. Also defaultCollectFn and defaultDescribeFn are added to standardise behaviour for unsupported platforms.
As this is my first contribution, I would really appreciate any feedback you may have. Thank you. @bwplotka @ArthurSens @kakkoyun