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

[receiver/k8scluster] fix node resource attributes #30641

Merged

Conversation

povilasv
Copy link
Contributor

@povilasv povilasv commented Jan 17, 2024

Description:

  • Remove os.version, which wasn't released yet.
  • add os.type

Link to tracking Issue:

#30342

Testing:

  • unit tests
    Documentation:
  • generated

@povilasv povilasv force-pushed the k8scluster/fix-resource-attributes branch from 768fab2 to c5110da Compare January 17, 2024 11:41
@povilasv povilasv marked this pull request as ready for review January 17, 2024 11:58
@povilasv povilasv requested a review from a team January 17, 2024 11:58
@@ -22,9 +22,12 @@ resourceMetrics:
- key: os.description
value:
stringValue: Ubuntu 22.04.1 LTS
- key: os.version
- key: os.kernel.version
Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm, but how about we add it to the semantics-convention and see if the group has any recommendations?

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.

I agree with opening a PR to add it to the semantic conventions. I'd also like to get this PR merged before the next release so we don't start publishing a value for os.version incorrectly. I am ok adding a new attribute that isnt stable in the semconv yet since it is off by default, but I dont like implementing a well-defined semconv incorrectly

# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
Copy link
Member

Choose a reason for hiding this comment

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

Given that os.version is not released yet, it's better to update the existing changelog entry that adding another one on top

@povilasv povilasv force-pushed the k8scluster/fix-resource-attributes branch 2 times, most recently from 717b66d to 2dc7440 Compare January 18, 2024 08:35
@povilasv povilasv force-pushed the k8scluster/fix-resource-attributes branch from 0d82c1d to 543e3ea Compare January 18, 2024 09:18
@povilasv
Copy link
Contributor Author

For now I removed os.version, will work on adding os.kernel.version to semantic conventions.

So this PR adds os.type, and changed changelog for previous PR to only add os.description

@dmitryax dmitryax merged commit 974aa42 into open-telemetry:main Jan 18, 2024
85 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 18, 2024
@povilasv povilasv deleted the k8scluster/fix-resource-attributes branch January 19, 2024 08:01
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants