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

fileexporter/receiver: refactor fileexporter and read compressed json in filereceiver #22021

Conversation

moh-osman3
Copy link
Contributor

Description:
[WIP] not working

This PR:

  • refactors fileexporter to use a compressed writer
  • adds support for logs and traces in filereceiver
  • adds support to read compressed json in filereceiver

@atoulme
Copy link
Contributor

atoulme commented May 17, 2023

Before you push too much into this, please know this PR's scope is too big for review. Please break it in 2.

Comment on lines +100 to +118
if cfg.Compression == "zstd" {
if fw, err := zstd.NewWriter(file); err == nil {
// flushing the compressed writer every second.
go func() {
for {
time.Sleep(1 * time.Second)
if fw.Flush() != nil {
return
}
}
}()

return &lineWriter{
file: bufio.NewWriter(fw),
}
}
}

return &lineWriter{
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 👍

Comment on lines +142 to +157
if cfg.Compression == "zstd" {
if fw, err := zstd.NewWriter(file); err == nil {
// flushing the compressed writer every second.
go func() {
for {
time.Sleep(1 * time.Second)
if fw.Flush() != nil {
return
}
}
}()
return &fileWriter{
file: bufio.NewWriter(fw),
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes compression to be per-stream not per-record, which I think is OK as long as the reader (a) has never supported this, (b) will support this after your change.

@moh-osman3
Copy link
Contributor Author

@moh-osman3 moh-osman3 closed this May 18, 2023
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.

3 participants