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

doc: Link hubble metrics to L7 visibility #13923

Merged
merged 1 commit into from
Nov 9, 2020
Merged

doc: Link hubble metrics to L7 visibility #13923

merged 1 commit into from
Nov 9, 2020

Conversation

mandarjog
Copy link
Contributor

Please ensure your pull request adheres to the following guidelines:

@mandarjog mandarjog requested a review from a team as a code owner November 6, 2020 09:29
@mandarjog mandarjog requested a review from rolinh November 6, 2020 09:29
@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 Nov 6, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Nov 6, 2020
sayboras
sayboras reviewed Nov 6, 2020
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for your PR. One suggestion as per below.

@gandro has a very nice explanation on similar topic here #13738 as well. FYI. Maybe if you feel the current docs is missing some points, we can incorporate into the relevant section.

Documentation/operations/metrics.rst Outdated
@@ -115,6 +115,10 @@ section for the full list of available metrics and their options.
The port of the Hubble metrics can be configured with the
``hubble.metrics.port`` Helm value.

Note: Hubble emits http metrics only for pods that have the
Copy link
Member

Choose a reason for hiding this comment

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

maybe like below (e.g. use Note section)

Suggested change
Note: Hubble emits http metrics only for pods that have the
.. Note::
Hubble emits http metrics only for pods that have the
``io.cilium.proxy-visibility`` annotation. Refer to
:ref:`Layer 7 Protocol Visibility <proxy_visibility>` for details.

Copy link
Member

Choose a reason for hiding this comment

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

I think using .. Note:: as suggested by @sayboras is a good idea.

I would phrase the sentence slightly differently however to avoid mentioning the specific label required as this is all explained with more details in the link.

What about something like this?

L7 metrics, such as HTTP, are only emitted for pods which have layer 7 protocol visibility enabled. Refer to :ref:Layer 7 Protocol Visibility <proxy_visibility> for details.

@sayboras sayboras added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Nov 6, 2020
@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 Nov 6, 2020
@maintainer-s-little-helper
Copy link

Commit d4bad2748951b004cc63e5dea339b4fec1886fb6 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 the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 9, 2020
@mandarjog mandarjog changed the title Link hubble metrics to L7 visibility doc: Link hubble metrics to L7 visibility Nov 9, 2020
@maintainer-s-little-helper
Copy link

Commit d4bad2748951b004cc63e5dea339b4fec1886fb6 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

@mandarjog
Copy link
Contributor Author

@sayboras thanks for point to #13738, I think the only issue is discoverability.
organization home » Network Policy » Layer 7 Protocol Visibility does not make sense since.
I expected to find the doc under home » Monitoring & Metrics

@mandarjog
Doc: Link hubble metrics to L7 visibility.
0423d8f 
Signed-off-by: Mandar U Jog <mjog@google.com>
@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 Nov 9, 2020
rolinh
rolinh approved these changes Nov 9, 2020
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.

Thanks for your contribution!

@rolinh rolinh added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 9, 2020
sayboras
sayboras approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM

@aanm aanm merged commit 53f35fb into cilium:master Nov 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.6 Nov 30, 2020
@gandro
Copy link
Member

gandro commented Nov 30, 2020

Marked for backport, as this remains a common pitfall.

This was referenced Nov 30, 2020
Merged
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.6 Nov 30, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.6 Dec 2, 2020
This was referenced Dec 4, 2020
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

@sayboras sayboras sayboras approved these changes

Assignees

@rolinh rolinh

Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.0
Done
1.8.6
Backport done to v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mandarjog @gandro @rolinh @sayboras @joestringer @aanm