-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[processor/resourcedetection] Add os.version
resource attribute
#38087
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @avilevy18!
@avilevy18 you need to gofmt the files and also add a change log entry: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-a-changelog-entry |
You might need to rebase on @Aneurysm9 do you mind taking a look at this PR when you have a chance? |
processor/resourcedetectionprocessor/internal/system/metadata.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address my review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
os.version: | ||
description: The os.version | ||
type: string | ||
enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is confusing for me as to why some of these attributes are enabled by default and why others are not.
I found #21482 which adds some context for atleast host.id
which is reasonable.
IMO though, the os.version
though reasonably should be enabled here by default but I'm okay with the earlier suggestion of making this enabled by default as a separate change that we mark as breaking in the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think of this also as a way to test things out for some users before we turn this on by default. This is all the more true for metrics, resource attributes can create time series recreations which can be painful if users are not aware of it.
…en-telemetry#38087) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Adding the `os.version` resource attribute to system `resourcedetection` processor <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes: open-telemetry#38059 <!--Describe what testing was performed and which tests were added.--> #### Testing Updated unit tests. <!--Describe the documentation added.--> #### Documentation N/A <!--Please delete paragraphs that you did not use before submitting.--> --------- Co-authored-by: Antoine Toulme <[email protected]>
Description
Adding the
os.version
resource attribute to systemresourcedetection
processorLink to tracking issue
Fixes: #38059
Testing
Updated unit tests.
Documentation
N/A