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

[processor/k8sattr] Use RFC3339 format for creation timestamp value #24016

Merged

Conversation

bryan-aguilar
Copy link
Contributor

@bryan-aguilar bryan-aguilar commented Jul 6, 2023

Description: This PR changes the format used for the k8s.pod.start_time value. Previously the .String() method was used but documentation for that says it should only be used for debugging. Instead use .MarshalText() which formats in RFC3339.

I have listed this as a breaking change because it is possible that end users are making assertions on the format of this timestamp value.

Timestamp output:
before: 2023-07-10 12:34:39.740638 -0700 PDT m=+0.020184946
after 2023-07-10T12:39:53.112485-07:00
Link to tracking Issue: n/a

Testing: Updated unit tests.

Documentation: Add blurb at bottom of readme

@bryan-aguilar bryan-aguilar requested a review from a team July 6, 2023 15:35
@github-actions github-actions bot added the processor/k8sattributes k8s Attributes processor label Jul 6, 2023
@github-actions github-actions bot requested a review from rmfitzpatrick July 6, 2023 15:35
@TylerHelmuth
Copy link
Member

@bryan-aguilar can you include in your description a before and after example of how the change affects the timestamp.

@bryan-aguilar
Copy link
Contributor Author

bryan-aguilar commented Jul 10, 2023

@TylerHelmuth done, thanks for the suggestion. Do you think this should be in the changelog entry also?

@TylerHelmuth
Copy link
Member

@bryan-aguilar ya in the subtext section

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

This component is ubiquitous enough that we probably need to follow our deprecation process and utilize feature gates for this breaking change. @dmitryax what do you think?

@dmitryax
Copy link
Member

This component is ubiquitous enough that we probably need to follow our deprecation process and utilize feature gates for this breaking change. @dmitryax what do you think?

Sounds good to me. @bryan-aguilar can you please run it by a feature gate and show a warning if this attribute is enabled?

@bryan-aguilar
Copy link
Contributor Author

Yup, will do.

@bryan-aguilar
Copy link
Contributor Author

@dmitryax Pod start time is enabled by default which would lead to us always showing a warning. Is this in line with what you had in mind?

@dmitryax
Copy link
Member

Pod start time is enabled by default which would lead to us always showing a warning. Is this in line with what you had in mind?

It is enabled by default, but we don't need to show the warning for users specifying extract::metadata explicitly without k8s.pod.start_time

@bryan-aguilar
Copy link
Contributor Author

@TylerHelmuth @dmitryax I have added a feature gate for this change. Indicating in the warning that the default behavior will be changed in v0.83.0. From what I could tell there was not a better mechanism to signal this other than a logging statement.

.chloggen/k8sattr_timestampformat.yaml Outdated Show resolved Hide resolved
.chloggen/k8sattr_timestampformat.yaml Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/README.md Outdated Show resolved Hide resolved
@TylerHelmuth TylerHelmuth merged commit aeb0bc9 into open-telemetry:main Jul 12, 2023
@github-actions github-actions bot added this to the next release milestone Jul 12, 2023
@bryan-aguilar bryan-aguilar deleted the k8sattr/timestampformat branch July 12, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/k8sattributes k8s Attributes processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants