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 remaining ECS attributes for file namespace #914

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

trisch-me
Copy link
Contributor

Add in this PR remaining attributes from ECS for file namespace

Merge requirement checklist

@trisch-me trisch-me requested review from a team April 9, 2024 16:19
@trisch-me
Copy link
Contributor Author

I want to bump this PR to get attention

@trisch-me
Copy link
Contributor Author

Please take a look into this PR if there are more questions about these fields or we can proceed?

@trisch-me
Copy link
Contributor Author

Just a reminder that this one is ready for approvals :)

model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
@trisch-me
Copy link
Contributor Author

@thompson-tomo I'm not opposed to create another PR after this one to add that particular field, or you can create it, they are independent, but I wouldn't like to increase scope of this PR as it lays here for quite a long time already and I hope it will be merged soon.

@trisch-me
Copy link
Contributor Author

@open-telemetry/semconv-security-approvers
@lmolkova @joaopgrassi please take a look

joaopgrassi
joaopgrassi previously approved these changes Jul 22, 2024
@joaopgrassi joaopgrassi dismissed their stale review July 23, 2024 08:39

Re-reading the discussions after the meeting on monday

@trisch-me
Copy link
Contributor Author

@lmolkova @joaopgrassi the owner comment is resolved. Also I have created additional pr to soften our guidelines for such cases
#1272

model/registry/file.yaml Outdated Show resolved Hide resolved
Primary Group ID (GID) of the file.
stability: experimental
examples: ["1000"]
- id: file.group
Copy link
Contributor

Choose a reason for hiding this comment

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

group has name and id, please consider renaming to file.group.name to reflect what this attribute describes and also allow future extensibility

model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
model/registry/file.yaml Show resolved Hide resolved
model/registry/file.yaml Outdated Show resolved Hide resolved
This attribute might not be supported by some file systems — NFS, FAT32, in embedded OS, etc.
stability: experimental
examples: ['2021-01-01T12:00:00Z']
- id: file.changed
Copy link
Member

Choose a reason for hiding this comment

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

file.change and file.modified can be easily confused 🤔 . These are part of ECS, right? Have you experienced any problems with these so far?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In ECS we have acronyms such as ctime and mtime and we didn't have there any problems (at least I'm not aware). Here I have changed it to be more descriptive

Copy link
Contributor

Choose a reason for hiding this comment

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

please move+rename this file to model/file/registry.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs more approvals
Status: Ready to be Merged
Development

Successfully merging this pull request may close these issues.

7 participants