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

Expand Hubble-exporter with filters and field mask #26379

Merged
merged 6 commits into from Jul 26, 2023
Merged

Expand Hubble-exporter with filters and field mask #26379

merged 6 commits into from Jul 26, 2023

Conversation

AwesomePatrol
Copy link
Contributor

@AwesomePatrol AwesomePatrol commented Jun 20, 2023 •
edited

Add support for filters and field mask to Hubble-exporter

This is prerequisite for implementation of #25508

Add an option to specify a filters and field mask for hubble-exporter

@AwesomePatrol AwesomePatrol requested a review from a team as a code owner June 20, 2023 15:21
@maintainer-s-little-helper
Copy link

Commit addb5970199f276f48a2e9e6d070bca8d7f551e0 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 20, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jun 20, 2023
@AwesomePatrol AwesomePatrol marked this pull request as draft June 20, 2023 15:22
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 20, 2023
@AwesomePatrol AwesomePatrol mentioned this pull request Jun 21, 2023
Merged
@AwesomePatrol AwesomePatrol mentioned this pull request Jul 3, 2023
Closed
@AwesomePatrol AwesomePatrol marked this pull request as ready for review July 4, 2023 08:45
@AwesomePatrol AwesomePatrol changed the title Draft: Expand Hubble-exporter with filters Expand Hubble-exporter with filters and field mask Jul 4, 2023
@AwesomePatrol
Copy link
Contributor Author

Do you think that I should also add new fields to config-map/command-line to configure new options of the exporter?

If yes, how should they be specified? Is JSON for raw filters ok?

@AwesomePatrol AwesomePatrol force-pushed the hubble-exporter-expanded branch 2 times, most recently from fb5b570 to be4f44c Compare July 5, 2023 09:56
@AwesomePatrol AwesomePatrol mentioned this pull request Jul 5, 2023
Closed
@chancez
Copy link
Contributor

chancez commented Jul 5, 2023

@AwesomePatrol yes, we should add some flags for this. JSON is fine.

And for adding to the configmap, that's also probably a good idea as well.

@AwesomePatrol AwesomePatrol requested review from a team as code owners July 6, 2023 08:07
@squeed squeed added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jul 7, 2023
@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 Jul 7, 2023
thorn3r
thorn3r approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@thorn3r thorn3r left a comment

Choose a reason for hiding this comment

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

agent changes lgtm 👍

asauber
asauber approved these changes Jul 8, 2023
View reviewed changes
Copy link
Member

@asauber asauber left a comment

Choose a reason for hiding this comment

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

CLI-related changes LGTM. Appreciate the clear variable names and parsing logic.

@lambdanis lambdanis added the sig/hubble Impacts hubble server or relay label Jul 11, 2023
chancez
chancez reviewed Jul 20, 2023
View reviewed changes
Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

Overall the approach LGTM. What do you think about adding some documentation to go with the export feature?

@AwesomePatrol
Copy link
Contributor Author

What do you think about adding some documentation to go with the export feature?

Sure, I can do that, but as there is a different PR relying on this can we have it done separately? Feel free to create an issue and assign it to me so that I won't forget.

@chancez chancez mentioned this pull request Jul 20, 2023
Closed
@chancez
Copy link
Contributor

chancez commented Jul 20, 2023

Created #26970 since we have existing flags + the new options that all need documenting.

@chancez
Copy link
Contributor

chancez commented Jul 21, 2023

/test

@AwesomePatrol
Copy link
Contributor Author

/test

@AwesomePatrol
Copy link
Contributor Author

/ci-e2e
/ci-ginkgo
/ci-aks

@aanm
Copy link
Member

aanm commented Jul 25, 2023

/test

@AwesomePatrol
Copy link
Contributor Author

AwesomePatrol commented Jul 25, 2023 •
edited

I think the same tests failed again, but it is hard to tell if it may be the fault of this change or tests. Are any of the failing tests configuring Hubble exporter? If not, I don't think it should affect them.

Could you please take a look?

@AwesomePatrol
Add filter support to Hubble-exporter
a70a490 
Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
@AwesomePatrol
Move fieldmask to separate package
a3736b5 
Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
@AwesomePatrol
Allocate fields when needed in fieldmask to avoid panics
90f831b 
Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
@AwesomePatrol
Add field mask support to Hubble-exporter
5fe77ba 
Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
@AwesomePatrol
Add Hubble exporter config options for fieldmask and filters
b77ea0d 
Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
@AwesomePatrol
Add benchmark of Hubble exporter
b32c3f9 
goos: linux
goarch: amd64
pkg: github.com/cilium/cilium/pkg/hubble/exporter
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
BenchmarkExporter-24 100000 13998 ns/op 1838 B/op 38 allocs/op

Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
@AwesomePatrol
Copy link
Contributor Author

/test

@aanm aanm merged commit 7adae92 into cilium:main Jul 26, 2023
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chancez chancez chancez approved these changes

@asauber asauber asauber approved these changes

@thorn3r thorn3r thorn3r approved these changes

@kaworu kaworu Awaiting requested review from kaworu

@michi-covalent michi-covalent Awaiting requested review from michi-covalent

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. 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
7 participants
@AwesomePatrol @chancez @aanm @asauber @thorn3r @squeed @lambdanis