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

https://issues.redhat.com/browse/ACM-15420 #7277

Open
wants to merge 6 commits into
base: 2.12_stage
Choose a base branch
from
+49 −42
Open

https://issues.redhat.com/browse/ACM-15420 #7277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
67effe2
https://issues.redhat.com/browse/ACM-15420
dockerymick Nov 22, 2024
8cf9769
https://issues.redhat.com/browse/ACM-15420
oafischer Nov 25, 2024
2edbda3
https://issues.redhat.com/browse/ACM-15420
oafischer Nov 25, 2024
e0ae7b8
https://issues.redhat.com/browse/ACM-15420
oafischer Nov 25, 2024
9edf03b
Apply suggestions from code review
dockerymick Dec 2, 2024
6ae51ef
Update observability_enable.adoc
dockerymick Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions observability/observability_enable.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,9 +574,13 @@ Update the `multicluster-observability-operator` resource by setting `enableMetr
spec:
imagePullPolicy: Always
imagePullSecret: multiclusterhub-operator-pull-secret
observabilityAddonSpec: # The ObservabilityAddonSpec defines the global settings for all managed clusters which have observability add-on enabled
enableMetrics: false #indicates the observability addon push metrics to hub server
observabilityAddonSpec: <1>
enableMetrics: false <2>
workers: <3>
----
<1> Define the global settings for all managed clusters that have the observability add-on enabled.
oafischer marked this conversation as resolved.
Show resolved Hide resolved
<2> Used to indicate if the observability add-on is enabled to push metrics to hub cluster server.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly we aren't supposed to use callouts for additional information, they are only for variable replacements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oafischer The additional information here is used to explain why the parameters are used. Since last release, I have used callouts as a solution to remove hashtags out of YAMLs.

Copy link
Contributor

@oafischer oafischer Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm OK with the suggestions you provide further down but we might need to bring this up in a meeting since I've been tasked with removing callouts that aren't about replacing variables in the past. Thanks for finding a compromise in the meantime. I'll approve after merging the suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oafischer ok cool, we can discuss in the team channel before I merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is I think not something we need to stress over, as I see teams use these callouts for a number of things now that I look at it. I don't agree with overusage, or even some of the writing here--or using two at one time for the same line, but see that they are used here by the telco/ocp writer:

https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/edge_computing/ztp-preparing-the-hub-cluster#ztp-preparing-the-ztp-git-repository_ztp-preparing-the-hub-cluster

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe when we first started using them, because we didn't before, we had guidance to only use them for variables but I lean towards using them to replace any #comments-that-clutter-the-code.

Copy link
Contributor

Choose a reason for hiding this comment

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

See here where the same product doc isn't clean and the code is cluttered with comments:

https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/networking/networking-operators#viewing-stats-collected-kubernetes-nmtate-op_k8s-nmstate-operator

Copy link
Contributor

@swopebe swopebe Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But I will say that since you have punctuation, these need to be full sentences.

Used to indicate if the observability add-on is enabled to push metrics to hub cluster server. (not complete)

Also, we agreed that unless in code, Observability is proper. You can do either way here. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@swopebe thanks for your comments and examples. I have updated the file with complete sentence and capitalized "observability" where needed. Awaiting review from @saswatamcode

oafischer marked this conversation as resolved.
Show resolved Hide resolved
<3> Used to list worker nodes into the metric collector process to shard federate requests made to Prometheus on your hub cluster. Then the metric collector sends sperate remote-write requests to Thanos on your hub cluster.
oafischer marked this conversation as resolved.
Show resolved Hide resolved

[#disabling-observability-on-a-single-cluster]
=== Disabling observability on a single cluster
Expand Down
2 changes: 2 additions & 0 deletions release_notes/acm_whats_new.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ For other Application topics, see link:../applications/app_management_overview.a

* You can now use the _Advanced search_ option from the console by selecting the *Advanced search* drop-down button. Specify your query and receive results that match the exact strings that you enter and range-based search parameters. See link:../console/search_console.adoc#search-customization[Search customization and configurations].

* Use the new `workers` parameter in the `ObservabilityAddOn` custom resource definition to add more worker nodes into the metric collector procress to shard federate requests made to your hub cluster. See link:../observability/observability_enable.adoc#enabling-observability-service[Enabling the observability service].

See link:../observability/observe_environments_intro.adoc#observing-environments-intro[Observability service introduction].

[#governance-whats-new]
Expand Down