[receiver/file] Initial implementation #6712
Conversation
atoulme
force-pushed
the
new_file_receiver
branch
6 times, most recently
from
6e7ec9b
to
ffbc713
Compare
There was a problem hiding this comment.
The code looks sane, but I have a few comments:
- it would have been nice to have followed the contribution guidelines, by splitting this into several PRs, to make reviewing easier (other reviewers might still require this from you)
- we are just about to decide that new components need a sponsor, who will be responsible for doing code reviews -- do you have an approver who'd be open to sponsor this component?
- related to the point above: did you discuss adding this with a maintainer/approver already?
|
I think a receiver like this is great but I have reservations since OTLP/JSON is unstable. I am worried that if we accept this people will start storing files in this format and will be very disappointed when we break the format. Yes, the Anyone willing to spend time and resolve open-telemetry/opentelemetry-specification#1443 before we move forward with this PR? |
jpkrohling
changed the title
atoulme
force-pushed
the
new_file_receiver
branch
from
ada694d
to
36c65b5
Compare
|
Thanks for the review. I am happy to break this PR into several. I am sure we can find an approver when this component is closer to merge. No, I didn't discuss this with an approver or maintainer yet. @tigrannajaryan I am going to review the issue and will apply myself to help resolve the stability issues. |
atoulme
force-pushed
the
new_file_receiver
branch
from
36c65b5
to
307dcbf
Compare
|
|
||
| func createLogsReceiver(_ context.Context, settings component.ReceiverCreateSettings, configuration config.Receiver, logs consumer.Logs) (component.LogsReceiver, error) { | ||
| readLogs := func(path string) error { | ||
| fileHandle, _ := os.Open(path) |
There was a problem hiding this comment.
The error returned here needs to be handled.
I worry that the file could be still be open by another application and cause issues that way.
There was a problem hiding this comment.
I am not a fan of lambdas within a function since it make it harder to provide debug information back to the user and being able to unit test them in isolation is problematic due to being coupled with a method that may not allow for unit testing.
There was a problem hiding this comment.
True, that's a miss. I can rework the lambda as a separate method, sure thing.
As for the file being opened by something else, etc, there's no guarantee in anything here. The file can be deleted, chmoded, moved, truncated, appended. It's going to be exactly as good as the operator makes it. This particular filereceiver is not very smart and we can spell that out in the docs if you like.
For concurrent access and live updates, we can work on kafka/redis/ queues.
There was a problem hiding this comment.
Is it right to assume that an exporter writing to the listened folder will be able to atomically write to it ?
There was a problem hiding this comment.
No, I'm not particularly trying to be good about it right now.
Sounds good, please ping me here when it happens. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Can someone please reopen this PR? |
|
I can't reopen the PR (which is weird, I thought admins should be able to). |
|
That would be really great to get it merged |
Description:
New receiver able to read pipeline data stored as JSON in files in a watched directory.
Testing:
Unit tests
Documentation:
README and example doc.