opentelemetry-collector-contrib-8267 Correcting 5m and 15m normalized… #8392
Conversation
|
@shree007 Please add a test case and a changelog entry |
|
Got it. I will add
…On Sun, 13 Mar, 2022, 9:18 pm Dmitrii Anoshin, ***@***.***> wrote:
@shree007 <https://github.com/shree007> Please add a test case and a
changelog entry
—
Reply to this email directly, view it on GitHub
<#8392 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADG76LLOGODLQTE4BLBZR73U7YL7NANCNFSM5QTP2K4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
<open-telemetry/opentelemetry-collector-contrib/pull/8392/c1066128759@
github.com>
|
| @@ -77,8 +77,8 @@ func (s *scraper) scrape(_ context.Context) (pdata.Metrics, error) { | |||
| if s.config.CPUAverage { | |||
| divisor := float64(runtime.NumCPU()) | |||
| avgLoadValues.Load1 = avgLoadValues.Load1 / divisor | |||
| avgLoadValues.Load5 = avgLoadValues.Load1 / divisor | |||
| avgLoadValues.Load15 = avgLoadValues.Load1 / divisor | |||
| avgLoadValues.Load5 = avgLoadValues.Load5 / divisor | |||
There was a problem hiding this comment.
Can you add a unit test to verify this change / regressions?
Edit: Sorry realized I repeated @dmitryax's comment
There was a problem hiding this comment.
Respectfully, there is a unit test: it verifies the data is present.
Leaving the code wrong to berate the contributor for not working out how to make a test that calculates a 15 minute average seems like an unnecessary harm.
…ntrib into opentelemetry-collector-contrib-8267
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
I will start to work on it from tomorrow.
…On Sun, Apr 3, 2022 at 10:45 AM github-actions[bot] < ***@***.***> wrote:
This PR was marked stale due to lack of activity. It will be closed in 14
days.
—
Reply to this email directly, view it on GitHub
<#8392 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADG76LK2KP2MOJR57HZCJR3VDESQHANCNFSM5QTP2K4A>
.
You are receiving this because you were mentioned.Message ID:
<open-telemetry/opentelemetry-collector-contrib/pull/8392/c1086778600@
github.com>
|
…ntrib into opentelemetry-collector-contrib-8267
The test must be incorrect if if didn't catch this issue. Please update the test and make sure it's failing without your fix. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Any news on this PR? Looks like a pretty straightforward fix. The current unit test simply looks that data exists and not that the data is correct. Perhaps the test suite should run for 15 minutes to generate enough data points to test the average? |
|
@sodabrew help would be appreciated. Feel free to take it if from here if you have a chance |
…ntrib into opentelemetry-collector-contrib-8267
|
|
shree007
force-pushed
the
opentelemetry-collector-contrib-8267
branch
from
f4ef8a3
to
e8aa34d
Compare
|
Looks like this didn't make the 0.51 release. |
|
@sodabrew there is multiple test is being failed, I need to look into it. Please feel free to take over if you have bandwidth. |
|
@shree007 Could you simply rebase this PR to current master? The test failures look unrelated to your change. It would be nice if @codeboten could merge this for 0.52 tomorrow @shree007 Are you able to complete the CLA? If not, then I don't mind resubmitting my own changes to effect the same fix. |
|
The test for this that could even have been updated was just disabled in #10030 |
|
Sure.
…On Wed, 25 May, 2022, 8:40 am Aaron Stone, ***@***.***> wrote:
The test for this that could even have been updated was just disabled in
#10030
<#10030>
—
Reply to this email directly, view it on GitHub
<#8392 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADG76LKBV44V44XUFTK25FTVLWK27ANCNFSM5QTP2K4A>
.
You are receiving this because you were mentioned.Message ID:
<open-telemetry/opentelemetry-collector-contrib/pull/8392/c1136676346@
github.com>
|
|
Got my version landed in #10301! Thanks again for this PR that got it started! Please do try a current build to double-check that it works for you |
|
Closing this, as it appears to have been addressed by #10301 |
Description:
wrong 5m and 15m normalized load - using 1m normalized load and normalizing it again
Link to tracking Issue: #8267