process_collector: Add Platform-Specific Describe for processCollector #1625
+188
−26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
bwplotka
approved these changes
Sep 16, 2024
| @@ -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?
| 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?
| @@ -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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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