[receiver/k8s_cluster] Sync allocatable metric names with the latest semconv #42587
Conversation
odubajDT
requested review from
a team,
ChrsMark,
TylerHelmuth and
dmitryax
as code owners
| ## Feature Gates | ||
| ### `k8scluster.allocatableNamespace.enabled` |
There was a problem hiding this comment.
Thank's @odubajDT for pushing this forward!
Shall we start using a generic feature gate for all these changes that aim to align the k8s components with the k8s semantic conventions?
We can start using the feature gate described at https://opentelemetry.io/docs/specs/semconv/non-normative/k8s-migration/ even though the Semantic Conventions are not stable yet.
@dmitryax @TylerHelmuth WDYT?
There was a problem hiding this comment.
A global feature gate may potentially enable features, that users do not want to enable, therefore I would rather stick to the concept of having a feature gate per feature, but I am opened for opinions
There was a problem hiding this comment.
The idea here is to have a single feature gate for porting SemConv to Collector k8s components, i.e. semconv.k8s.enableStable. In this we handle SemConv porting with one single gate making the migration easier. At least that's the plan we have defined from SemConv's stability groups and documented at https://opentelemetry.io/docs/specs/semconv/non-normative/k8s-migration/
There was a problem hiding this comment.
I understand, but if multiple features in multiple different components are enabled/disabled by a single feature gate, it may cause problems for the users. For example: User want to enable feature X in component Z, but with the feature gate, also feature B in component A is enabled. Therefore user enables behavior, that his infrastructure is not prepared yet.
There was a problem hiding this comment.
The feature gate semconv.k8s.enableStable will only enable stable Semantic Conventions for k8s across the k8s components. Can this be considered a different feature? In any case the counterpart of this feature gate will be the semconv.k8s.enableLegacy and there will be a period where the dual scheme will be needed for a smooth migration. Metrics can always be switched on/off individually.
I think this will give us less overhead during the migration period, because having to maintain multiple feature gates will be a pain both from code perspective but also for the users that would need to handle them differently in their configurations.
However, I'd be open to hear if we can do better than what the decided single feature gate suggests. Also interested to hear what @dmitryax @TylerHelmuth @braydonk think on this.
There was a problem hiding this comment.
Makes sense, thanks for the clarification! Waiting for @dmitryax @TylerHelmuth @braydonk opinions on which approach to go with here
There was a problem hiding this comment.
pinging @dmitryax @TylerHelmuth @braydonk
There was a problem hiding this comment.
We already have an option to enable/disable particular metrics in the user config. I don't see why we would need to provide this granularity additionally. I find a option to double-publish more important. So I agree with @ChrsMark, we should have two feature gates to enable/disable old and new metrics. Maybe receiver.k8scluster.metrics.disableLegacy and receiver.k8scluster.metrics.enableStable? I'm not sure if we have any naming standard here.
While the receiver.k8scluster.metrics.enableStable is in alpha we can keep adding new metrics to it
There was a problem hiding this comment.
At https://opentelemetry.io/docs/specs/semconv/non-normative/k8s-migration/ we have came up with semconv.k8s.disableLegacy and semconv.k8s.enableLegacy, but we can still change it and make the gates receiver specific.
There was a problem hiding this comment.
thanks for the opinions, should be adapted!
receiver/k8sclusterreceiver/internal/node/testdata/expected_allocatable.yaml
Outdated
Show resolved
Hide resolved
odubajDT
force-pushed
the
k8scluster-allocatable
branch
from
d48b9d7 to
93eaf01
Compare
…emconv Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
odubajDT
force-pushed
the
k8scluster-allocatable
branch
from
93eaf01 to
894ee22
Compare
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
not stale |
odubajDT
changed the title
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
| enabled: true | ||
| k8s.node.allocatable.memory: | ||
| enabled: true | ||
| k8s.node.allocatable.pods: |
There was a problem hiding this comment.
Apologies for the back and forth here. This is going to change again with open-telemetry/semantic-conventions#2822 because it needs to be aligned with SemConv project. I think the rest of the metrics should change too i.e. k8s.node.cpu.allocatable (I'll send a follow-up PR after). In this, I suggest we wait until we have the semconv part ready, which hopefully will be soon.
There was a problem hiding this comment.
thanks for taking care of it!
6 participants
Description
Sync allocatable metric names with the latest semconv. This change is being handled with a feature gate, so users have a clean transition phase
Link to tracking issue
Fixes #40708
Documentation
README