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.

Sign up for GitHub

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

Jump to bottom

[receiver/hostmetrics] Collect cpu utilization metric #7130

Merged
merged 9 commits into from
Mar 7, 2022
Merged

[receiver/hostmetrics] Collect cpu utilization metric #7130

merged 9 commits into from
Mar 7, 2022

Conversation

rubenruizdegauna
Copy link
Contributor

Description:

This PR adds system.cpu.utilization to hostmetricsreceiver

Link to tracking Issue:
#6221

Testing:
Tests added for existing CPU scrapper and for the added CPU Utilization calculator

Documentation:
New metrics added to Documentation

@rubenruizdegauna rubenruizdegauna requested a review from a team January 11, 2022 09:26
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 11, 2022 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ rubenruizdegauna (b6cb0a4ff9007a8f925353a6a161c87b689d07eb)

bogdandrutu
bogdandrutu reviewed Jan 12, 2022
View reviewed changes
receiver/hostmetricsreceiver/internal/scraper/cpuscraper/cpu_scraper.go Outdated
Comment on lines 73 to 79
cpuUtilizations, err := s.ucal.Calculate(now, cpuTimes)
if err != nil {
return md, scrapererror.NewPartialScrapeError(err, metricsLen)
}
for _, cpuUtilization := range cpuUtilizations {
s.recordCPUUtilization(now, cpuUtilization)
}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like unnecessary slice of utilizations, should we simply pass a func pointer recordCPUUtilization to the ucal so it calls into the right func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way the calculator just handles the part of calculation and the scrapper it is the responsible of recording making it more decoupled and easier to test. Still, it adds this overhead so I guess that it is a trade-off.

If you think that this kind of overhead needs to be avoided as much as possible, I can work on it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think @bogdandrutu ?

Copy link
Member

Choose a reason for hiding this comment

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

I do believe that unnecessary overhead is good to be removed.

Copy link
Contributor Author

@rubenruizdegauna rubenruizdegauna Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

👍 I added the recorder to the calculator to record it as soon as it is calculated. 8593685

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think @bogdandrutu ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bogdandrutu 👋 I already moved the recordCPUUtilization functionality inside the calculator to avoid overhead. Let me know if this looks better now 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bogdandrutu , reading the specification discussion about having both metrics, seems that it can make sense to have both metrics and this could be a good candidate to be disabled by default. I have made another commit with this last change.

What do you think?

@tigrannajaryan
Copy link
Member

Otherwise we will be sending essentially the same data in two formats by default which is not desirable.

Is system.cpu.time just the cumulative value of system.cpu.utilization or there are other differences? If they track the exact same value with same attributes and the only difference is that one is cumulative and the other is delta do we really need both?

@tigrannajaryan
Copy link
Member

I also asked for clarification in the spec open-telemetry/semantic-conventions#647

@jlegoff
Copy link
Contributor

jlegoff commented Jan 24, 2022

Is system.cpu.time just the cumulative value of system.cpu.utilization or there are other differences? If they track the exact same value with same attributes and the only difference is that one is cumulative and the other is delta do we really need both?

@tigrannajaryan my understanding was that system.cpu.utilization was a gauge, not a delta. It can be derived from system.cpu.time by computing the delta and dividing it by the elapsed time.

@rubenruizdegauna rubenruizdegauna mentioned this pull request Jan 24, 2022
Closed
2 tasks
@tigrannajaryan
Copy link
Member

@tigrannajaryan my understanding was that system.cpu.utilization was a gauge, not a delta. It can be derived from system.cpu.time by computing the delta and dividing it by the elapsed time.

Sounds good. I think the spec needs a clarification but the metrics are indeed different.

@dmitryax
Copy link
Member

@rubenruizdegauna sorry for the delay. Can you please rebase, otherwise it looks good to me

@rubenruizdegauna
Copy link
Contributor Author

@rubenruizdegauna sorry for the delay. Can you please rebase, otherwise it looks good to me

NP :) Rebased and all tests ✅ in my fork.

Thanks @dmitryax !!

rubenruizdegauna
rubenruizdegauna commented Feb 22, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member

I think this PR is good to be merged. @bogdandrutu WDYT?

@rubenruizdegauna
Copy link
Contributor Author

Hi @bogdandrutu , this PR has been approved and checks are ✅ . Do you think it can be merged? Thanks!

@jpkrohling jpkrohling changed the title collect cpu utilization metric in host metrics receiver [receiver/hostmetrics] Collect cpu utilization metric Mar 7, 2022
@jpkrohling jpkrohling merged commit 92bcf6a into open-telemetry:main Mar 7, 2022
@rubenruizdegauna rubenruizdegauna deleted the host_metrics_receiver-cpu_utilization branch November 9, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitryax dmitryax dmitryax approved these changes

@alanwest alanwest alanwest left review comments

@bogdandrutu bogdandrutu bogdandrutu left review comments

@jlegoff jlegoff jlegoff left review comments

Assignees

@bogdandrutu bogdandrutu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rubenruizdegauna @tigrannajaryan @jlegoff @dmitryax @bogdandrutu @alanwest @jpkrohling