Skip to content

metrics: add structured format for Hubble metrics and options. #34849

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
Merged
merged 1 commit into from
Sep 26, 2024
Merged

metrics: add structured format for Hubble metrics and options. #34849

merged 1 commit into from
Sep 26, 2024

Conversation

rectified95
Copy link
Contributor

Re-submitting PR #32294 now that the requested Hubble metrics server test was added in #34325
All the work was authored by @vakalapa. In the commit I added, I addressed the small comments left on it and simplified the test code by removing some indirection when passing in ContextOptionConfig.

This change adds a structured internal format for metrics and metrics options, instead of the existing string representation.

First PR in the series to implement:
https://github.com/cilium/design-cfps/blob/main/hubble/CFP-30788-managing-hubble-metrics-cardinality.md

CFP: cilium/design-cfps#29

Will follow up with code to derive this config from a new ConfigMap.

Checklist:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

@rectified95 rectified95 requested review from a team as code owners September 13, 2024 07:01
@rectified95 rectified95 requested a review from rolinh September 13, 2024 07:01
@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 13, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 13, 2024
@rectified95 rectified95 changed the title Vakalapa/structuredctxopts Add structured format for Hubble metrics and options. Sep 13, 2024
@rectified95 rectified95 changed the title Add structured format for Hubble metrics and options. metrics: add structured format for Hubble metrics and options. Sep 13, 2024
@rectified95
Copy link
Contributor Author

FYI @chancez

chancez
chancez reviewed Sep 16, 2024
View reviewed changes
chancez
chancez requested changes Sep 16, 2024
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 LGTM. My only thought is DynamicMetricsConfig could probably be renamed since it's really the usage that determines whether or not it's dynamic. It's a bit confusing to have ParseStaticMetricsConfig return a DynamicMetricsConfig.

@maintainer-s-little-helper
Copy link

Commit 48e2e03 does not match "(?m)^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 the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 16, 2024
@rectified95 rectified95 force-pushed the vakalapa/structuredctxopts branch from 48e2e03 to a87326a Compare September 16, 2024 21:03
@maintainer-s-little-helper
Copy link

Commit 48e2e03 does not match "(?m)^Signed-off-by:".

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

@rectified95 rectified95 force-pushed the vakalapa/structuredctxopts branch from a87326a to de6d5fe Compare September 16, 2024 21:19
@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 Sep 16, 2024
@rectified95 rectified95 force-pushed the vakalapa/structuredctxopts branch from de6d5fe to b17c643 Compare September 16, 2024 22:27
@rectified95 rectified95 marked this pull request as draft September 16, 2024 22:43
@rectified95 rectified95 force-pushed the vakalapa/structuredctxopts branch 3 times, most recently from 65b2200 to 2045187 Compare September 17, 2024 17:56
@rectified95 rectified95 marked this pull request as ready for review September 17, 2024 18:11
@rolinh rolinh requested a review from chancez September 18, 2024 09:35
@rectified95 rectified95 force-pushed the vakalapa/structuredctxopts branch from 2045187 to 105f0af Compare September 18, 2024 14:56
@rectified95
Copy link
Contributor Author

rectified95 commented Sep 18, 2024 •
edited
Loading

@rolinh Squashed all commits.

@rectified95
Copy link
Contributor Author

@chancez let me know if this is good

@chancez
Copy link
Contributor

chancez commented Sep 20, 2024

/test

@rectified95
Copy link
Contributor Author

Can we re-run the 3 failed jobs for extra signal?

Known flake: in Conformance Runtime: #34731
New issue, similar to other cases of Hubble Relay not starting: #34994
Maybe existing issue with Hubble Relay connection, this PR in Cilium E2E Upgrade: #24805 (comment)

@qmonnet qmonnet added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. sig/hubble labels Sep 24, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 24, 2024
rolinh
rolinh approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing. Changes lgtm.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 26, 2024
@rolinh rolinh added this pull request to the merge queue Sep 26, 2024
Merged via the queue into cilium:main with commit 2c0ef77 Sep 26, 2024
63 checks passed
This was referenced Oct 2, 2024
Merged
Merged
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

@chancez chancez chancez approved these changes

Assignees
No one assigned
Labels
area/metrics Impacts statistics / metrics gathering, eg via Prometheus. kind/community-contribution This was a contribution made by a community member. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rectified95 @chancez @rolinh @qmonnet @vakalapa