Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add k8s.container.status.current_waiting_reason resource attribute #997

Merged
merged 4 commits into from
May 24, 2024

Conversation

povilasv
Copy link
Contributor

@povilasv povilasv commented May 2, 2024

Fixes #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 povilasv marked this pull request as ready for review May 2, 2024 12:38
@povilasv povilasv requested review from a team as code owners May 2, 2024 12:38
@povilasv povilasv changed the title add k8s.container.status.waiting_reason resource attribute add k8s.container.status.current_waiting_reason resource attribute May 9, 2024
@povilasv povilasv force-pushed the k8s-waiting-status branch 2 times, most recently from 0d95649 to f756629 Compare May 9, 2024 07:09
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@povilasv
Copy link
Contributor Author

Would appreciate a review here 🙇

Maybe @TylerHelmuth or @lmolkova you could take a look 👀

@ChrsMark
Copy link
Member

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

@joaopgrassi joaopgrassi assigned joaopgrassi and unassigned reyang May 24, 2024
@joaopgrassi joaopgrassi merged commit c53f76f into open-telemetry:main May 24, 2024
12 checks passed
@trask
Copy link
Member

trask commented May 24, 2024

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

@joaopgrassi
Copy link
Member

joaopgrassi commented May 28, 2024

@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?

@ChrsMark
Copy link
Member

ChrsMark commented May 28, 2024

#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?

@joaopgrassi
Copy link
Member

Yeah, probably as events I'd say.

@povilasv
Copy link
Contributor Author

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?

@joaopgrassi
Copy link
Member

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.

@povilasv
Copy link
Contributor Author

povilasv commented Jun 3, 2024

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.

@ChrsMark
Copy link
Member

ChrsMark commented Jun 3, 2024

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?

@povilasv
Copy link
Contributor Author

povilasv commented Jun 3, 2024

Agree

@lmolkova
Copy link
Contributor

lmolkova commented Jun 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

add k8s.container.status.waiting_reason resource attribute
7 participants