[chore][receiver/k8scluster] Add pods field to allocatable_types_to_report parameter options
#39924
Conversation
odubajDT
requested review from
dmitryax,
TylerHelmuth,
ChrsMark and
a team
as code owners
odubajDT
force-pushed
the
k8sclusterreceiver-pods-allocable
branch
from
be0d2bb to
430eeab
Compare
There was a problem hiding this comment.
👍🏼
Could you file an issue for Semantic Conventions project as well? It seems we miss those from the list of existing metrics open-telemetry/semantic-conventions#1032.
Probably that's because they are not explicitly defined in the metadata.yaml.
|
…eport parameter options Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
odubajDT
force-pushed
the
k8sclusterreceiver-pods-allocable
branch
from
27fa221 to
3a48d97
Compare
| @@ -3017,6 +3073,11 @@ func (mb *MetricsBuilder) RecordK8sNamespacePhaseDataPoint(ts pcommon.Timestamp, | |||
| mb.metricK8sNamespacePhase.recordDataPoint(mb.startTime, ts, val) | |||
| } | |||
|
|
|||
| // RecordK8sNodeAllocatableDataPoint adds a data point to k8s.node.allocatable metric. | |||
| func (mb *MetricsBuilder) RecordK8sNodeAllocatableDataPoint(ts pcommon.Timestamp, val int64) { | |||
There was a problem hiding this comment.
I don't see any actual code that uses it. Is it expected?
There was a problem hiding this comment.
this is a generated code by mdatagen. Even if it's not used, the CI will fail if it's not present
There was a problem hiding this comment.
This is coming from https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/39924/files#r2116129944 right?
We would need to split the metrics in the metadata.yaml and then use the generated code instead of the custom one at
| k8s.node.allocatable: | ||
| enabled: false | ||
| description: The allocatable metrics of a particular Node. | ||
| unit: "" |
There was a problem hiding this comment.
Shouldnt this have a unit?
There was a problem hiding this comment.
since allocatable is a multi-datapoint metric (you can have allocatable memory, cpu, pods etc), it does not make sense to have a unit here
There was a problem hiding this comment.
In that case, shouldn't be different metrics? We hit the same issue in resourcequota metrics and we had to split: open-telemetry/semantic-conventions#2076 (comment)
There was a problem hiding this comment.
Actually that will be covered by open-telemetry/semantic-conventions#2267 anyways, right?
But can't we just split the metrics in the metadata.yaml already since those are already different metrics:
There was a problem hiding this comment.
if the code owners agree, I would wait until the semconv is clear and merged and afterwards the receiver can be adapted accordingly, anybody against this approach?
There was a problem hiding this comment.
I think we need to deprecate this approach that uses the allocatable_types_to_report setting to enable 5 different metrics and define each metric on its own properly.
For now we can just add support for pods and work on a follow-up to make it happen properly through the metadata.yaml etc.
Right now the addition of the k8s.node.allocatable in the metadata.yaml does not make any sense as https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/39924/files#r2109946850 point out, right?
We will also need to coordinate this change with open-telemetry/semantic-conventions#2267, probably first ship the SemConv part and then introduce the metrics one by one in metadata.yaml, and eventually deprecate the allocatable_types_to_report setting.
| k8s.node.allocatable: | ||
| enabled: false | ||
| description: The allocatable metrics of a particular Node. | ||
| unit: "" |
There was a problem hiding this comment.
Actually that will be covered by open-telemetry/semantic-conventions#2267 anyways, right?
But can't we just split the metrics in the metadata.yaml already since those are already different metrics:
| @@ -3017,6 +3073,11 @@ func (mb *MetricsBuilder) RecordK8sNamespacePhaseDataPoint(ts pcommon.Timestamp, | |||
| mb.metricK8sNamespacePhase.recordDataPoint(mb.startTime, ts, val) | |||
| } | |||
|
|
|||
| // RecordK8sNodeAllocatableDataPoint adds a data point to k8s.node.allocatable metric. | |||
| func (mb *MetricsBuilder) RecordK8sNodeAllocatableDataPoint(ts pcommon.Timestamp, val int64) { | |||
There was a problem hiding this comment.
This is coming from https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/39924/files#r2116129944 right?
We would need to split the metrics in the metadata.yaml and then use the generated code instead of the custom one at
Link to tracking issue
Fixes #39923