semantic-conventions icon indicating copy to clipboard operation
semantic-conventions copied to clipboard

add k8s.container.status.current_waiting_reason resource attribute

Open povilasv opened this issue 1 year ago • 2 comments

Fixes https://github.com/open-telemetry/semantic-conventions/issues/996

Changes

Please provide a brief description of the changes here.

Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.

Merge requirement checklist

povilasv avatar May 02 '24 12:05 povilasv

Would appreciate a review here :bow:

Maybe @TylerHelmuth or @lmolkova you could take a look :eyes:

povilasv avatar May 15 '24 13:05 povilasv

@open-telemetry/semconv-k8s-approvers @open-telemetry/specs-semconv-approvers please take a look.

ChrsMark avatar May 15 '24 13:05 ChrsMark

sorry for the late comment, was just curious if there was any discussion already about this resource attribute being non-immutable?

trask avatar May 24 '24 13:05 trask

@trask The collector linked PR says:

FYI I've opened a PR on semconv for last terminated reason

But I see that this is not reflected in the attribute definition and can be confusing. @povilasv can you please clarify if this is the correct assumption of this attribute? Resource attributes are immutable. Is it even possible to know the "last terminated reason"? Or are we assuming to take the current state and that's it?

joaopgrassi avatar May 28 '24 08:05 joaopgrassi

https://github.com/open-telemetry/semantic-conventions/pull/968 introduced the k8s.container.status.last_terminated_reason. This one adds a different attribute. From my perspective these attributes represent "states" of the container. If those cannot be expressed as resource attributes because of the immutability constraint then what would be the right way to report them?

ChrsMark avatar May 28 '24 08:05 ChrsMark

Yeah, probably as events I'd say.

joaopgrassi avatar May 28 '24 15:05 joaopgrassi

Hey folks,

Did not know about immutability constraint on resource attributes. Sorry for this.

Looks like k8s.container.status.current_waiting_reason is not immutable. Also container.status.last_terminated_reason might be not immutable (Though it's still a question, as I think - once container is terminated it gets new container.id and is a new Entity)

But I think it would be best to model it as metric that tracks state changes. Also events are an option.

I guess we also should remove these two and model it differently.

How do I revert PRs/ deprecate the attributes?

povilasv avatar May 29 '24 06:05 povilasv

We don't allow removing things anymore and just deprecating, but since this wasn't released yet I think we can just remove it. What do you think @lmolkova ? The other one that was merged container.status.last_terminated_reason is part of the last release though, so that one cannot be removed and just deprecated.

joaopgrassi avatar Jun 03 '24 11:06 joaopgrassi

I don't think container.status.last_terminated_reason should be removed, as it's AFAIK immutable. Only new containers can get last terminated reason. I.E. container has to terminate in order for container.status.last_terminated_reason to change.

povilasv avatar Jun 03 '24 11:06 povilasv

I don't think container.status.last_terminated_reason should be removed, as it's AFAIK immutable.

While that's most probably true, I wonder if we should find a consistent way to model both of them. It could be weird to find k8s.container.status.last_terminated_reason being a resource attribute while k8s.container.status.current_waiting_reason being a metric or event?

ChrsMark avatar Jun 03 '24 13:06 ChrsMark

Agree

povilasv avatar Jun 03 '24 13:06 povilasv

We don't allow removing things anymore and just deprecating, but since this wasn't released yet I think we can just remove it.

it's ok to remove things that haven't been released - we know that nothing depends on it.

lmolkova avatar Jun 03 '24 23:06 lmolkova

I'm way late to the party, but I think we need to back out the resource-attribute portion of this change.

See: https://github.com/open-telemetry/semantic-conventions/issues/1181#issuecomment-2229168814 for rationale. If I had caught this earlier I would have blocked it earlier, so apologies for being late. At a minimum, this is still experimental, so we CAN deprecate the resource attribute.

jsuereth avatar Jul 15 '24 18:07 jsuereth

@jsuereth we didn't release this, removed in https://github.com/open-telemetry/semantic-conventions/pull/1115, so nothing to worry about :)

povilasv avatar Jul 16 '24 06:07 povilasv