Skip to content

Conversation

@virtuald
Copy link
Contributor

@virtuald virtuald commented Sep 1, 2018

I really enjoyed your writeup about the tailer package, and it's definitely the best pure go tail solution out there. The tailer package is immensely useful, it should be it's own project!

But, until that day, one nit is that it has a grok_exporter specific message in it, which is confusing if your application isn't grok_exporter...

Disclaimer: I haven't actually tested this code yet since I don't use grok_exporter itself, but this seems like the right fix.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 69.93% when pulling 465c5bc on virtuald:errmsg into 118f9f6 on fstab:master.

@fstab
Copy link
Owner

fstab commented Sep 3, 2018

Sounds like a good idea. However, the error is wrapped in the writeError() call in fileTailer.go, so if you call os.IsNotExist() in grok_exporter.go it will always be false. I think the best way to solve this would be to define a custom error type for the tail.Errors() channel. This type could define methods like IsFileNotFound() to get more info on what caused the error.

@virtuald
Copy link
Contributor Author

virtuald commented Sep 4, 2018

Could wrap os.PathError instead of defining a custom error type?

@fstab
Copy link
Owner

fstab commented Sep 5, 2018

I am not sure. We now have the message "failed to initialize file system watcher..." so the caller knows it's an initialization problem and not a runtime problem. I am not sure how we could keep that message if we use the os.PathError directly in writeError().

@fstab
Copy link
Owner

fstab commented Sep 11, 2018

I pushed an tailer.Error type where you can call Cause() to get the original error.

@fstab
Copy link
Owner

fstab commented Sep 11, 2018

Seems that I broke the Windows build. Need to check tomorrow.

@virtuald
Copy link
Contributor Author

Your solution seems fine to me, thanks!

@virtuald virtuald closed this Sep 12, 2018
@virtuald virtuald deleted the errmsg branch September 12, 2018 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants