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.

Sign up for GitHub

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

Jump to bottom

[chore][pkg/stanza] Unexport reader's file name field #27434

Merged
merged 1 commit into from
Oct 11, 2023
Merged

[chore][pkg/stanza] Unexport reader's file name field #27434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
[chore][pkg/stanza] Unexport reader's file name field
  • Loading branch information
@djaglowski
djaglowski committed Oct 6, 2023
commit a58692505b46c2ec0e8c6eb41a28b21d954b5ba0
10 changes: 5 additions & 5 deletions pkg/stanza/fileconsumer/internal/reader/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (f *Factory) build(file *os.File, m *Metadata, lineSplitFunc bufio.SplitFun
Config: f.Config,
Metadata: m,
file: file,
FileName: file.Name(),
fileName: file.Name(),
logger: f.SugaredLogger.With("path", file.Name()),
decoder: decode.New(f.Encoding),
lineSplitFunc: lineSplitFunc,
Expand All @@ -84,12 +84,12 @@ func (f *Factory) build(file *os.File, m *Metadata, lineSplitFunc bufio.SplitFun
}

// Resolve file name and path attributes
resolved := r.FileName
resolved := r.fileName

// Dirty solution, waiting for this permanent fix https://github.com/golang/go/issues/39786
// EvalSymlinks on windows is partially working depending on the way you use Symlinks and Junctions
if runtime.GOOS != "windows" {
resolved, err = filepath.EvalSymlinks(r.FileName)
resolved, err = filepath.EvalSymlinks(r.fileName)
if err != nil {
f.Errorf("resolve symlinks: %w", err)
}
Expand All @@ -100,12 +100,12 @@ func (f *Factory) build(file *os.File, m *Metadata, lineSplitFunc bufio.SplitFun
}

if f.Config.IncludeFileName {
r.FileAttributes[attrs.LogFileName] = filepath.Base(r.FileName)
r.FileAttributes[attrs.LogFileName] = filepath.Base(r.fileName)
} else if r.FileAttributes[attrs.LogFileName] != nil {
delete(r.FileAttributes, attrs.LogFileName)
}
if f.Config.IncludeFilePath {
r.FileAttributes[attrs.LogFilePath] = r.FileName
r.FileAttributes[attrs.LogFilePath] = r.fileName
} else if r.FileAttributes[attrs.LogFilePath] != nil {
delete(r.FileAttributes, attrs.LogFilePath)
}
Expand Down
14 changes: 9 additions & 5 deletions pkg/stanza/fileconsumer/internal/reader/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type Metadata struct {
type Reader struct {
*Config
*Metadata
FileName string
fileName string
logger *zap.SugaredLogger
file *os.File
lineSplitFunc bufio.SplitFunc
Expand Down Expand Up @@ -138,8 +138,8 @@ func (r *Reader) Delete() {
return
}
r.Close()
if err := os.Remove(r.FileName); err != nil {
r.logger.Errorf("could not delete %s", r.FileName)
if err := os.Remove(r.fileName); err != nil {
r.logger.Errorf("could not delete %s", r.fileName)
}
}

Expand Down Expand Up @@ -188,6 +188,10 @@ func min0(a, b int) int {
return b
}

func (r *Reader) NameEquals(other *Reader) bool {
return r.fileName == other.fileName
}

// ValidateOrClose returns true if the reader still has a valid file handle, false otherwise.
// If false is returned, the file handle should be considered closed.
//
Expand All @@ -201,14 +205,14 @@ func (r *Reader) ValidateOrClose() bool {
}
refreshedFingerprint, err := fingerprint.New(r.file, r.FingerprintSize)
if err != nil {
r.logger.Debugw("Closing unreadable file", zap.Error(err), zap.String(attrs.LogFileName, r.FileName))
r.logger.Debugw("Closing unreadable file", zap.Error(err), zap.String(attrs.LogFileName, r.fileName))
r.Close()
return false
}
if refreshedFingerprint.StartsWith(r.Fingerprint) {
return true
}
r.logger.Debugw("Closing truncated file", zap.String(attrs.LogFileName, r.FileName))
r.logger.Debugw("Closing truncated file", zap.String(attrs.LogFileName, r.fileName))
r.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a rename right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, not just a rename. It also corrects the behavior. It was previously described in the function description that this should happen but it wasn't happening yet. I mentioned the correction in the PR description though I'm not sure this is a user visible change in any case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I realized you may have been asking about whether this could be a renamed file. If so:

I don't think we can ascertain whether the file has been renamed. We know the file name as it was when it was first opened (because we called os.Open(filename)), but there is no way to refresh the name from the information we have.

return false
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/stanza/fileconsumer/roller_other.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ OUTER:
continue OUTER
}

if oldReader.FileName != newReader.FileName {
if !newReader.NameEquals(oldReader) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a change of behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. The equal check is moved into the function.

The inversion of the condition is met with a complimentary inversion of how we react to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, this was a change. It should only continue the inner loop if the file names do not match. I opened #27454 to make the change separately. I'll plan on rebasing this PR.

continue
}

Expand Down
Loading