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

Rename host metrics according to metrics spec #2311

Merged

Conversation

james-bebbington
Copy link
Member

@james-bebbington james-bebbington commented Dec 21, 2020

Link to tracking Issue: #2220

Description: Update all metric names / descriptions to match the specification.

This PR also adds a couple of minor features for Linux that have recently been added to gopsutil:

  1. Major Page Faults label on the page faults metric
  2. Processes Created metric

Note there are some major breaking changes in this PR:

  • swap scraper changed to paging scraper, config will need to be updated accordingly
  • Various metric names and formats changed to be in line with the spec

@james-bebbington james-bebbington requested a review from a team as a code owner December 21, 2020 04:48
@project-bot project-bot bot added this to In progress in Collector Dec 21, 2020
metric := pdata.NewMetric()
metric.SetName("system.processes.blocked")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only place where I haven't followed the spec. The spec actually says this should be "system.processes.count", but I think it may make more sense to get the spec changed as otherwise this will very confusing with the "per process" metrics.

FYI @aabmass

@@ -12,13 +12,13 @@
// See the License for the specific language governing permissions and
// limitations under the License.

// +build windows
Copy link
Member Author

Choose a reason for hiding this comment

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

Make Linux the special case instead of Windows, as other systems including Darwin seem to use the same standard names as Windows.

@codecov
Copy link

codecov bot commented Dec 21, 2020

Codecov Report

Merging #2311 (77abe90) into master (d47baee) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2311      +/-   ##
==========================================
+ Coverage   92.04%   92.07%   +0.03%     
==========================================
  Files         272      273       +1     
  Lines       15337    15358      +21     
==========================================
+ Hits        14117    14141      +24     
+ Misses        840      837       -3     
  Partials      380      380              
Impacted Files Coverage Δ
receiver/hostmetricsreceiver/factory.go 86.84% <ø> (ø)
...rnal/scraper/processesscraper/processes_scraper.go 77.77% <ø> (ø)
...iver/internal/scraper/diskscraper/disk_metadata.go 100.00% <100.00%> (ø)
...nternal/scraper/diskscraper/disk_scraper_others.go 100.00% <100.00%> (ø)
...l/scraper/filesystemscraper/filesystem_metadata.go 100.00% <100.00%> (ø)
...nternal/scraper/networkscraper/network_metadata.go 100.00% <100.00%> (ø)
...internal/scraper/networkscraper/network_scraper.go 97.91% <100.00%> (+0.02%) ⬆️
...receiver/internal/scraper/pagingscraper/factory.go 100.00% <100.00%> (ø)
.../internal/scraper/pagingscraper/paging_metadata.go 100.00% <100.00%> (ø)
...nal/scraper/pagingscraper/paging_scraper_others.go 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d47baee...77abe90. Read the comment docs.

@james-bebbington james-bebbington force-pushed the hostmetrics-names branch 2 times, most recently from 2087b9e to a8ba26e Compare December 21, 2020 05:28
metric.SetName("system.processes.running")
metric.SetDescription("Total number of running processes.")
metric.SetUnit("1")
metric.SetName("system.processes.created")
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this one should be added to the spec as well

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, @aabmass can you take this as an action item?

Copy link
Member

Choose a reason for hiding this comment

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

Collector automation moved this from In progress to Reviewer approved Dec 21, 2020
@bogdandrutu bogdandrutu merged commit 36c22b9 into open-telemetry:master Dec 21, 2020
Collector automation moved this from Reviewer approved to Done Dec 21, 2020
punya added a commit to punya/opentelemetry.io that referenced this pull request Jun 15, 2021
flands pushed a commit to open-telemetry/opentelemetry.io that referenced this pull request Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants