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

Add tcpstat connection states per source/dest ports #2785

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tomachine
Copy link

@tomachine tomachine commented Aug 22, 2023

Added new metric to tcpstats collector, as requested here #2490.

Signed-off-by: Tomáš Kadaně <tomas.kadane@cdn77.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.

Nice, LGTM!

@@ -75,6 +84,11 @@ func NewTCPStatCollector(logger log.Logger) (Collector, error) {
"Number of connection states.",
[]string{"state"}, nil,
), prometheus.GaugeValue},
descPort: typedDesc{prometheus.NewDesc(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably not be a separate metric, but rather change the labels for the existing metric depending on the flag.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to implement this. The new metric is working per source/dest port, that is being described by labels "type" and "port". Based on your comment, the solution should be one metric that based on the flags should contain the labels or not? The prometheus library won't allow me to do so. Can you please explain further, how you mean it? thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SuperQ Hi, can I ask you about the implementation? We need to have the metrics and I would like to finish the MR, thanks:)

@frittentheke
Copy link
Contributor

This is (will be) really cool @tomachine !

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.

None yet

4 participants