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

feat: Make net_(tcp|udp) read limit configurable #622

Closed
wants to merge 3 commits into from

Conversation

rexagod
Copy link
Contributor

@rexagod rexagod commented Mar 22, 2024

Defaults to 4GiB otherwise, as earlier.

Fixes: #364

Defaults to 4GiB otherwise, as earlier.

Fixes: prometheus#364
Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
@rexagod
Copy link
Contributor Author

rexagod commented May 12, 2024

Rebased. Ready for reviews.

@discordianfish
Copy link
Member

I think we should just implement the ptr function ourselves, it's just this:

// To returns a pointer to the given value.
func To[T any](v T) *T {
	return &v
}

@rexagod rexagod force-pushed the 364 branch 3 times, most recently from a371e9f to f38e372 Compare May 13, 2024 15:43
Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
Copy link
Member

@discordianfish discordianfish left a 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
Copy link
Member

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 <rexagod@gmail.com>
Copy link
Member

@SuperQ SuperQ left a 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.

See: prometheus/node_exporter#2322

@rexagod
Copy link
Contributor Author

rexagod commented May 14, 2024

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 readLimit to FS, but that made little sense as it wasn't at all cohesive).

Seeing that we use netlink as a replacement for this, I'm fine with closing out this PR and adding a deprecation notice. Thank you for all the details.

@rexagod rexagod closed this May 14, 2024
rexagod added a commit to rexagod/procfs that referenced this pull request May 14, 2024
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
prometheus#622 (review).

Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
rexagod added a commit to rexagod/procfs that referenced this pull request May 14, 2024
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
prometheus#622 (review).

Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
rexagod added a commit to rexagod/procfs that referenced this pull request May 14, 2024
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
prometheus#622 (review).

Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
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.

Make net tcp/udp read limit configurable
3 participants