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

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

Closed
wants to merge 3 commits into from
Closed

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

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

@rexagod
feat: Make net_(tcp|udp) read limit configurable
7409825 
Defaults to 4GiB otherwise, as earlier.

Fixes: prometheus#364
Signed-off-by: Pranshu Srivastava <[email protected]>
@rexagod
Copy link
Contributor Author

rexagod commented May 12, 2024 •
edited
Loading

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
discordianfish
discordianfish approved these changes May 13, 2024
View reviewed changes
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

SuperQ
SuperQ reviewed May 13, 2024
View reviewed changes
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?

SuperQ
SuperQ requested changes May 14, 2024
View reviewed changes
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
@rexagod
chore: deprecate NewTCPx methods
cc9a45b 
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
prometheus#622 (review).

Signed-off-by: Pranshu Srivastava <[email protected]>
rexagod added a commit to rexagod/procfs that referenced this pull request May 14, 2024
@rexagod
chore: deprecate NewTCPx methods
75858bc 
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
prometheus#622 (review).

Signed-off-by: Pranshu Srivastava <[email protected]>
@rexagod rexagod mentioned this pull request May 14, 2024
Merged
rexagod added a commit to rexagod/procfs that referenced this pull request May 14, 2024
@rexagod
chore: deprecate NewTCPx methods
e3757ce 
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
prometheus#622 (review).

Signed-off-by: Pranshu Srivastava <[email protected]>
SuperQ pushed a commit that referenced this pull request Jun 3, 2024
@rexagod
chore: deprecate NewTCPx methods (#640)
661a63e 
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
#622 (review).

Signed-off-by: Pranshu Srivastava <[email protected]>
jritter pushed a commit to jritter/procfs that referenced this pull request Jul 15, 2024
@rexagod @jritter
chore: deprecate NewTCPx methods (prometheus#640)
fe3eabd 
Deprecate TCP APIs in favor of `github.com/mdlayher/netlink`.

Refer:
prometheus#622 (review).

Signed-off-by: Pranshu Srivastava <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ requested changes

@discordianfish discordianfish discordianfish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make net tcp/udp read limit configurable
3 participants
@rexagod @discordianfish @SuperQ