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

[chore] move log to registry #908

Merged
merged 8 commits into from
Apr 22, 2024
Merged

Conversation

trisch-me
Copy link
Contributor

While moving log attributes to the registry I have noticed that we have a repetition of file namespace for some of the log attributes. I have replaced them with fields from file namespace and deprecated old attributes.

Merge requirement checklist

@trisch-me trisch-me requested review from a team April 8, 2024 20:53
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.

@trisch-me do we know the impact of this breaking change (log.file.path -> file.path)?

I know for example that this would affect the recombine parser for k8s container logs.

Shall we collect all the possible affected implementations and evaluate the impact before merging this one?

schema-next.yaml Outdated Show resolved Hide resolved
@trisch-me
Copy link
Contributor Author

@ChrsMark can you help me identify all places I should check?

So far I have checked open-telemetry/opentelemetry-collector-contrib and found 1 instance of using file.name in code

@ChrsMark
Copy link
Member

A search within the whole org give some additional impacted projects: https://github.com/search?q=org%3Aopen-telemetry+log.file.name&type=code&p=1.

Most of them are language specific implementations.
Similar for log.file.path.

In particular k8s container logs would break all users collecting container logs (in cri-o format) from k8s using Opentelemetry's helm charts presets. @TylerHelmuth feel free to correct me here and if you have any ideas of how this should be handled by the helm charts project.

I'm not opposing to that change, just want to ensure that we will properly handle it.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

I'm not convinced this change is necessary or correct.

The intention behind the log.* namespace is to convey information about the mechanism of log transmission, which may need to exist simultaneously with other attributes describing a file.

For example, an application which tries to open a file and fails may emit a structured log e.g.

level: error
message: failed to open file
file.name: foo.csv
time: ...

In such a case, the purpose of log.file.name is distinct from file.name. I think we should reserve this dedicated namespace for the purpose of describing log media separately from log content.

@trisch-me
Copy link
Contributor Author

@djaglowski I have a question: do you think there might be in this structured log information about some other file? If this comes in one message, information is connected, so file attributes will be connected to the log.

Is it different from your point of view?

@djaglowski
Copy link
Member

do you think there might be in this structured log information about some other file?

Yes, in the example above I'm trying to show that an application which needs to access foo.csv may write a log describing this interaction to a log file which is called something else, say application.log. Both of these file names have a distinct semantic meaning - one to describe the application event, the other to describe how the event was transmitted.

@trisch-me
Copy link
Contributor Author

trisch-me commented Apr 16, 2024

Now I got your point. I agree with you in this case it will be different files. This will be resolved as soon as we will implement re-usage of the namespaces within other namespaces, so in that case we will have file.* on the top level and log.file.* both pointing to the same file namespace. But for now I will revert this change and will add a TODO remark

thank you for bringing it out

@trisch-me
Copy link
Contributor Author

I have reverted changes, please have a look

@trisch-me trisch-me changed the title [BREAKING] move log to registry; use file namespace for some log attributes [chore] move log to registry; use file namespace for some log attributes Apr 17, 2024
@trisch-me trisch-me changed the title [chore] move log to registry; use file namespace for some log attributes [chore] move log to registry Apr 17, 2024
@trisch-me trisch-me added the Skip Changelog Label to skip the changelog check label Apr 17, 2024
@joaopgrassi joaopgrassi merged commit 7f6876d into open-telemetry:main Apr 22, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Label to skip the changelog check
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants