Skip to content
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

feat: make metrics build specific #237

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

thugrock7
Copy link
Contributor

Description

CPU and memory metrics which are added, making them linux specific. For other platforms using no-op

Testing

Tested on Mac, linux

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 22.38806% with 52 lines in your changes missing coverage. Please review.

Project coverage is 58.61%. Comparing base (e8225ec) to head (0572059).

Files Patch % Lines
...entation/opentelemetry/internal/metrics/metrics.go 0.00% 27 Missing ⚠️
...on/opentelemetry/internal/metrics/linux_metrics.go 37.50% 23 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
- Coverage   59.89%   58.61%   -1.29%     
==========================================
  Files          70       71       +1     
  Lines        2364     2375      +11     
==========================================
- Hits         1416     1392      -24     
- Misses        863      903      +40     
+ Partials       85       80       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@varkey98
Copy link
Contributor

done reviewing, missed this earlier

@varkey98
Copy link
Contributor

@shashank11p @puneet-traceable pls take a look as well

@varkey98
Copy link
Contributor

Adding some context here: The reason is that /proc/pid/stat file is only available on linux and trying to read from that file on other platforms results in an error

@varkey98
Copy link
Contributor

@thugrock7 Pls test it on windows as well

return lm.cpuSecondsTotal
}

func (lm *linuxMetrics) processStatsFromPid(pid int) (*processStats, error) {
Copy link
Contributor

@puneet-traceable puneet-traceable Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no tests for this function? It's fine for this but in general for testing such scenarios

  1. either file should become a parameter to the function and then you can create custom files to test.
  2. can pass an io.Reader to the function to read file. (*os.File) implements an io.Reader. During test you can use a mock version.

@varkey98 varkey98 requested a review from tim-mwangi July 25, 2024 12:41
@thugrock7
Copy link
Contributor Author

Tested running this by http-server/client scenario on linux, Mac, windows. Works fine
Also confirmed the exported metrics on TPA in linux, mac os.

@varkey98 varkey98 merged commit ca8ae6e into hypertrace:main Jul 25, 2024
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants