-
Notifications
You must be signed in to change notification settings - Fork 314
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
feat: Make net_(tcp|udp)
read limit configurable
#622
Conversation
Defaults to 4GiB otherwise, as earlier. Fixes: prometheus#364 Signed-off-by: Pranshu Srivastava <[email protected]>
Rebased. Ready for reviews. |
I think we should just implement the ptr function ourselves, it's just this:
|
a371e9f
to
f38e372
Compare
Signed-off-by: Pranshu Srivastava <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
utils.go
Outdated
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package procfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an internal/util
package. Perhaps this should go there?
Signed-off-by: Pranshu Srivastava <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like how this changes the public interface of the functions. Using a nil
to specify "Use the default" is not great for library usability.
IMO, I don't think we should change this. Maybe better we should mark this function as not recommended or even deprecated (with no intention to remove).
In the node_exporter we stopped using these functions in favor of reading the data via netlink (github.com/mdlayher/netlink
). This is a much more reliable method as well as it doesn't have the kernel locking issues that necessitate the reading method we use here.
Initially, I did have doubts around changing the public signature, even though we don't guarantee any backward compatibilities, but I decided to go forward with the patch anyway since that was the only way of addressing the issue (there was the possibility of adding Seeing that we use |
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`. Refer: prometheus#622 (review). Signed-off-by: Pranshu Srivastava <[email protected]>
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`. Refer: prometheus#622 (review). Signed-off-by: Pranshu Srivastava <[email protected]>
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`. Refer: prometheus#622 (review). Signed-off-by: Pranshu Srivastava <[email protected]>
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`. Refer: #622 (review). Signed-off-by: Pranshu Srivastava <[email protected]>
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`. Refer: prometheus#622 (review). Signed-off-by: Pranshu Srivastava <[email protected]>
Defaults to 4GiB otherwise, as earlier.
Fixes: #364