Allow globs in FPM unix socket paths #7089
Conversation
Co-Authored-By: Steven Soroka <ssoroka78@gmail.com>
Doing it on every gather would simplify things for me, as no further action would be needed to monitor/stop monitoring dynamically managed pools. Doing it on startup wouldn't be a deal breaker: I'd simply have to reload [restart?] Telegraf every time a pool is added or removed, along with the reload of the FPM daemon that I already perform. One advantage of doing it on every gather is that it won't cause pauses in the data collection of unrelated metrics because Telegraf had to be restarted to rebuild the FPM socket list. |
plugins/inputs/phpfpm/phpfpm.go
Outdated
| if len(paths) == 0 { | ||
| _, err := os.Stat(pattern) | ||
| if os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("Socket doesn't exist '%s': %s", pattern, err) | ||
| } | ||
| return nil, err |
There was a problem hiding this comment.
I'm not sure I understand this correctly, would this check for files that contain '*' characters? I feel like this is probably not needed, this seems very edge case, but either way we should consider if it should be an error to not match a socket. In the other plugins using globs, not having matches is generally not an error but this decision it can make it difficult to debug. If there are situations where it is normal to not match a socket, then it might be better to ignore the error.
There was a problem hiding this comment.
I did it this way to keep the previous behavior, which is to error out on nonexistent socket paths. Since filepath.Glob ignores any errors other than syntax ones, I've used os.Stat to get the appropriate error. The special handling of os.ErrNotExist is there because it's also done elsewhere in the plugin.
If you think we consider that not matching any sockets is not an error, this code would indeed be a bit cleaner.
There was a problem hiding this comment.
One example of where this handling is useful is when the unix socket path is in fact not a glob, and the file does not exist. In that case, we'd get an empty slice of strings as a result from the glob, with no error, so this extra checking restores the error message one would get before glob support.
There was a problem hiding this comment.
Okay, let's continue to show an error when the socket doesn't exist. I think we should then remove the Stat call on L166, which doesn't seem useful anymore and anyway we will report an error afterwards connecting to the socket if it doesn't exist.
There was a problem hiding this comment.
Looking at the globpath module I noticed Match actually returns the given parameter when it's a static path, so the latest commit handles this case correctly now. I've also removed the redundant os.Stat call.
If the PR is OK now, do you want me to rebase before merging?
plugins/inputs/phpfpm/phpfpm.go
Outdated
| if len(paths) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
If we are planning to show an error if a glob doesn't have any matches, then I think we need to do it here.
There was a problem hiding this comment.
Hmm ok, my thinking was that since globpath can diferentiate static paths from patterns, to allow non-matching globs, but to error on static paths. But it's probably better to be consistent.
plugins/inputs/phpfpm/phpfpm.go
Outdated
| // globpath.Match() returns the given argument when it's a static path, | ||
| // i.e., not a glob pattern. In that case, ensure it exists. | ||
| if len(paths) == 1 && paths[0] == pattern { | ||
| if _, err := os.Stat(paths[0]); err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("Socket doesn't exist '%s': %s", pattern, err) | ||
| } | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Since, as you mentioned, Match() will return the path if there are no special glob characters, I think we can remove this too. We will still display an error when we dial the socket.
There was a problem hiding this comment.
Done (this required a change to the unit tests due to the error now coming from Dial).
danielnelson
added
the
feat
|
Merged for 1.14, thanks! |
|
Thank you! |
This PR allows glob patterns to be used in the
unixsocketconfiguration. This is useful in environments where FPM pools are managed dynamically, so that the configuration file doesn't have to be edited every time a pool is created or deleted.Required for all PRs: