awsecscontainermetrics: update CpuUtilized metric calculation #1283
Conversation
|
@hossain-rayhan Unfortunately resource is resource - I think it's precisely the exporter layer that should deal with handling resource attributes correctly and not correct for the receiver to fill these in as labels. Apps send resource, so if the exporter isn't handling it correctly, then app metrics are already broken anyways. Do you mind scoping this PR down just to the percentage value change for CPUUtilized? |
|
@anuraaga , Yes, I removed metric label settings from this PR. Will create a separate PR with those changes. Please, let's move with |
There was a problem hiding this comment.
Is it safe to not check CPUReserved == 0 here?
There was a problem hiding this comment.
For a task, CPUReserved cannot be zero. If the value is not present, we calculate it from all the containers. And, in ECS task, you must need to set CPU Limit- either in task level or in container level.
There was a problem hiding this comment.
Cool - let's add a comment to the effect
There was a problem hiding this comment.
Let's use Previous instead of Pre in the code (not json which is fixed I suppose) for readability.
hossain-rayhan
changed the title
|
Thanks @anuraaga for the fast review. |
|
@hossain-rayhan the Datadog exporter issue was fixed on #1274, you can rebase/merge master to get the tests to pass |
…and update cpu calculations Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
hossain-rayhan
force-pushed
the
cpu-utilized
branch
from
31c2161 to
2f66404
Compare
|
cools. I rebased my code with master and all the tests passed. Thanks @mx-psi |
Signed-off-by: Bogdan Drutu <[email protected]>
4 participants
Description:
We had our internal discussion and we decided to report
CpuUtilizedmetric in percentage. We studied our new calculation and verified with domain expert.[Update] As the reviewer's suggestion, will create a separate PR for the following one.
Earlier we decided to add task and container metadata as resource attributes. However, CloudWatch
awsemfexporter reads dimensions only from metric labels. As this exporter is major target forawsecscontainermetricsreceiver, we need to add these metadata as metric labels. I discussed this in our OTel Metric SIG meeting. We also discussed internally and agreed to add them as metric labels as well.This change will address both of these issues.
Link to tracking Issue:
#1282
Testing:
Unit test added.