[chore] Clarify .count for updowncounters guidance #2317
Conversation
Signed-off-by: ChrsMark <[email protected]>
ChrsMark
force-pushed
the
updowncounter_guidance
branch
from
eccc9c6 to
1655619
Compare
ChrsMark
moved this from Untriaged
to Needs More Approval
in Semantic Conventions Triage
| Using `count` is not required for UpDownCounters when there is no meaningful namespace to preserve. | ||
| For instance, `k8s.deployment.desired_pods` is acceptable, as there is no need to use singular | ||
| `k8s.deployment.desired_pod.*` as a namespace. |
There was a problem hiding this comment.
wait, we've been actively aligning updowncounters to follow k8s.deployment.desired_pod.count pattern, why would we change direction now? it'd be even more confusing than today
There was a problem hiding this comment.
This comes from the discussion at #1800 (comment).
If we still want to enforce .count to all updowncounters then we should change the guidance accordingly to make this clear. However, I doubt that's sth we want to do now if what @trask shared at #1800 (comment) was the original motivation.
In any case, whatever we decide on we need to fix the guidance accordingly to avoid confusion.
ps: I won't be able to join the Mon SIG meeting but please share any decision here so I can update the PR accordingly :)
There was a problem hiding this comment.
I would be happy to completely drop this part of guidance 2 years ago, but we've been actively pushing everyone to follow it and implemented it in some stable conventions.
Changing or removing it now would mean some chaos and more inconsistencies to come. I'll try to prepare a list of metrics so we can evaluate how many irreversible decisions we've made so far.
To some extent it's a matter of taste and not a technical/rational decision to call something foo.bar.active_requests or foo.bar.active_request.count - it's like tab vs space - make decision once, add lint, and everyone should follow the repo policy. Having mixture is the worst situation to be in.
There was a problem hiding this comment.
stable plural metrics violating the current no-pluralization guidance (8, self-contained within .NET):
- aspnetcore.rate_limiting.active_request_leases
- aspnetcore.rate_limiting.queued_requests
- kestrel.active_connections
- kestrel.active_tls_handshakes
- kestrel.queued_connections
- kestrel.queued_requests
- kestrel.upgraded_connections
- signalr.server.active_connections
development metrics violating the current guidance (29, 24 of them as k8s):
- db.client.connection.pending_requests
- http.client.active_requests
- http.client.open_connections
- http.server.active_requests
- k8s.cronjob.active_jobs
- k8s.daemonset.current_scheduled_nodes
- k8s.daemonset.desired_scheduled_nodes
- k8s.daemonset.misscheduled_nodes
- k8s.daemonset.ready_nodes
- k8s.deployment.available_pods
- k8s.deployment.desired_pods
- k8s.hpa.current_pods
- k8s.hpa.desired_pods
- k8s.hpa.max_pods
- k8s.hpa.min_pods
- k8s.job.active_pods
- k8s.job.desired_successful_pods
- k8s.job.failed_pods
- k8s.job.max_parallel_pods
- k8s.job.successful_pods
- k8s.replicaset.available_pods
- k8s.replicaset.desired_pods
- k8s.replicationcontroller.available_pods
- k8s.replicationcontroller.desired_pods
- k8s.statefulset.current_pods
- k8s.statefulset.desired_pods
- k8s.statefulset.ready_pods
- k8s.statefulset.updated_pods
- system.network.connections
metric following current guidance:
- 55 development
- 16 stable
I'd prefer the outcome that's is
- minimally disruptive
- does not make things more confusing
I have two ideas:
-
Just say to avoid pluralization (just because it's a common practice).
- Use count instead of pluralization for UpDownCounters + Do not pluralize UpDownCounter names
-
Just remove this part from the guidance. If pluralization is sometimes ok and sometimes not, just pick whatever name works, we don't need guidance.
Fixes #2306
Changes
This PR tries to clarfiy the guidance regarding the usage of
.countforUpDownCounters as it was described at #2306./cc @trask @lmolkova @braydonk
Merge requirement checklist
[chore]