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

hubble/metrics: Add source_ip/destination_ip labels to contextLabels #21322

Merged
merged 1 commit into from
Oct 5, 2022
Merged

hubble/metrics: Add source_ip/destination_ip labels to contextLabels #21322

merged 1 commit into from
Oct 5, 2022

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Sep 15, 2022 •
edited
Loading

Update hubble metrics to support exposing the source/destination IP as source_ip and destination_ip labels.

hubble/metrics: Add source_ip/destination_ip labels to contextLabels

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 15, 2022
@chancez
Copy link
Contributor Author

chancez commented Sep 15, 2022

PR is currently based on #21181

@kaworu kaworu force-pushed the pr/chancez/http_v2_metrics branch 3 times, most recently from cf51271 to 1a3ddba Compare September 26, 2022 07:59
@kaworu kaworu force-pushed the pr/chancez/http_v2_metrics branch 2 times, most recently from 4dfa6ca to e0b182a Compare September 28, 2022 14:10
Base automatically changed from pr/chancez/http_v2_metrics to master September 29, 2022 14:38
@rolinh rolinh force-pushed the pr/chancez/source_dest_ip_labels branch from aeaa09a to ad4ed3e Compare September 30, 2022 13:12
@rolinh rolinh added sig/hubble Impacts hubble server or relay release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Sep 30, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 30, 2022
@chancez chancez force-pushed the pr/chancez/source_dest_ip_labels branch from ad4ed3e to aaabfdd Compare October 3, 2022 15:52
@chancez chancez marked this pull request as ready for review October 3, 2022 17:02
@chancez chancez requested review from a team as code owners October 3, 2022 17:02
@chancez
Copy link
Contributor Author

chancez commented Oct 3, 2022

/test

@chancez chancez force-pushed the pr/chancez/source_dest_ip_labels branch from aaabfdd to a1efe23 Compare October 3, 2022 17:45
@chancez
Copy link
Contributor Author

chancez commented Oct 3, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.24-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Checks in-cluster KPR with L7 policy Tests NodePort with L7 Policy

Failure Output

FAIL: Request from testclient-cm9zv pod to service http://[fd04::11

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-4.19 so I can create one.

@chancez
Copy link
Contributor Author

chancez commented Oct 3, 2022

/test-1.24-4.19

@chancez
Copy link
Contributor Author

chancez commented Oct 3, 2022

/mlh new-flake Cilium-PR-K8s-1.24-kernel-4.19

@chancez
Copy link
Contributor Author

chancez commented Oct 4, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test-1.25-net-next

Job 'Cilium-PR-K8s-1.25-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig Check BPF masquerading with ip-masq-agent DirectRouting

Failure Output

FAIL: Failed to add ip route

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-net-next so I can create one.

@chancez chancez force-pushed the pr/chancez/source_dest_ip_labels branch from a1efe23 to 9ad385c Compare October 4, 2022 15:20
aditighag
aditighag reviewed Oct 4, 2022
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

I don't have context for this PR. It looks like a Hubble change primarily? I'm not sure what the context labels is as source and destination ips are already dumped in the flow logs.

@chancez
Copy link
Contributor Author

chancez commented Oct 4, 2022

I don't have context for this PR. It looks like a Hubble change primarily? I'm not sure what the context labels is as source and destination ips are already dumped in the flow logs.

I've updated the PR description. It's primarily about adding the IPs as labels into hubble metrics.

qmonnet
qmonnet approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

I've updated the PR description.

Generally speaking, please make sure to pass the context through commit logs too, they're easier to read than the PR description when working in the repository and browsing through the history of the file.

Change looks good, thanks!

aditighag
aditighag approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

I don't have context for this PR. It looks like a Hubble change primarily? I'm not sure what the context labels is as source and destination ips are already dumped in the flow logs.

I've updated the PR description. It's primarily about adding the IPs as labels into hubble metrics.

Ok, thanks. It might be useful to add a snippet of metrics dashboard/command line output to the PR/commit description. Same goes for the other hubble/metrics PR.

@chancez chancez force-pushed the pr/chancez/source_dest_ip_labels branch from 9ad385c to 5e01c42 Compare October 4, 2022 15:39
@chancez
Copy link
Contributor Author

chancez commented Oct 4, 2022

/test

@chancez
Copy link
Contributor Author

chancez commented Oct 5, 2022

/ci-multicluster

@rolinh rolinh added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 5, 2022
@chancez @rolinh
hubble/metrics: Add source_ip/destination_ip labels to contextLabels
fc6a4ab 
Update hubble metrics to support exposing the source/destination IP as
source_ip and destination_ip labels.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@rolinh rolinh force-pushed the pr/chancez/source_dest_ip_labels branch from 5e01c42 to fc6a4ab Compare October 5, 2022 09:35
@rolinh rolinh removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 5, 2022
@rolinh
Copy link
Member

rolinh commented Oct 5, 2022

Marking as ready to merge, CI was green prior to the rebase and the conflict was minor and due to #21320 changing the same line for the metrics labels.

@rolinh rolinh added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 5, 2022
@ti-mo ti-mo merged commit c6cbd83 into master Oct 5, 2022
@ti-mo ti-mo deleted the pr/chancez/source_dest_ip_labels branch October 5, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@qmonnet qmonnet qmonnet approved these changes

@aditighag aditighag aditighag approved these changes

@michi-covalent michi-covalent Awaiting requested review from michi-covalent michi-covalent was automatically assigned from cilium/sig-hubble

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chancez @rolinh @qmonnet @aditighag @ti-mo