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

Monitoring guide #2407

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Monitoring guide #2407

merged 1 commit into from
Oct 12, 2023

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Aug 31, 2023 •
edited

This is a port of the old "Monitoring Satellite" guide. Draft because:

  • still need to incorporate changes for RHEL8, Pulp3 etc
  • verify that the file structure matches current best practices
  • add guide to index
  • squash all commits

Please cherry-pick my commits into:

  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (planned Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13)
  • Foreman 3.4/Katello 4.6 (EL8 only)
  • Foreman 3.3/Katello 4.5 on EL7 & EL8 (Satellite 6.12 on EL8 only; orcharhino 6.4/6.5 on EL8 only)
  • Foreman 3.2/Katello 4.4 on EL7 & EL8
  • Foreman 3.1/Katello 4.3 on EL7 & EL8 (Satellite 6.11 EL7/8; orcharhino 6.3 on EL7/8)
  • We do not accept PRs for Foreman older than 3.1.

@github-actions
Copy link

github-actions bot commented Aug 31, 2023 •
edited

The PR preview for 1976061 is available at theforeman-foreman-documentation-preview-pr-2407.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@evgeni evgeni force-pushed the monitoring-guide branch 5 times, most recently from 442eb01 to 8f16d2f Compare August 31, 2023 09:44
ekohl
ekohl reviewed Aug 31, 2023
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Just some comments from a quick look.

guides/doc-Monitoring_Project/topics/proc_configuring-pcp-data-collection.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/proc_configuring-pcp-data-collection.adoc Outdated
+
[options="nowrap", subs="verbatim,quotes,attributes"]
----
# {foreman-installer} --foreman-telemetry-prometheus-enabled true
Copy link
Member

Choose a reason for hiding this comment

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

This runs the installer multiple times. Would it be good to only run it once, saving time?

Copy link
Member Author

Choose a reason for hiding this comment

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

FIXME: make the installer quicker 😝

@ehelms
Copy link
Member

ehelms commented Aug 31, 2023

Will we need a note somehow in the Upgrade guide about what to do if PCP monitoring is present?

@evgeni
Copy link
Member Author

evgeni commented Aug 31, 2023

Will we need a note somehow in the Upgrade guide about what to do if PCP monitoring is present?

Why? Because services and thus data might be unavailable? I don't think it's a problem, but didn't test this much yet.

@evgeni
Copy link
Member Author

evgeni commented Oct 10, 2023

@maximiliankolb @mjivraja @Lennonka @asteflova this is now ready for a review :)

maximiliankolb
maximiliankolb requested changes Oct 10, 2023
View reviewed changes
guides/doc-Monitoring_Project/master.adoc Show resolved Hide resolved
guides/doc-Monitoring_Project/master.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/con_metrics-overview.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/con_metrics-overview.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/con_performance-co-pilot.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/proc_installing-pcp-packages.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/proc_identifying-available-metrics.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/proc_identifying-available-metrics.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/proc_identifying-available-metrics.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/topics/proc_enabling-access-to-telemery-data-via-the-web-ui.adoc Outdated Show resolved Hide resolved
@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author Needs re-review and removed Waiting on contributor Requires an action from the author labels Oct 10, 2023
@evgeni evgeni force-pushed the monitoring-guide branch 3 times, most recently from 0cbb4a9 to 565ed01 Compare October 10, 2023 12:12
Lennonka
Lennonka requested changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@Lennonka Lennonka left a comment •
edited

Choose a reason for hiding this comment

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

Assuming that everything is technically correct, I'm adding a couple of suggestions.

I see that the guide might use significant structural improvements, which is kinda complex to describe step by step.
How about we merge it more-or-less "as is", then I would suggest the further structural improvements in a new PR, and we continue the review then?
@evgeni Would you be okay with that?

guides/common/attributes-titles.adoc Outdated Show resolved Hide resolved
guides/doc-Monitoring_Project/docinfo.xml Outdated
<title>Monitoring Red Hat Satellite</title>
<productname>Red Hat Satellite</productname>
<productnumber>6.14</productnumber>
<subtitle>Collecting metrics from Red Hat Satellite 6</subtitle>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<subtitle>Collecting metrics from Red Hat Satellite 6</subtitle>
<subtitle>Collect metrics from Satellite and analyze the metrics in an external/third-party monitoring application/tool</subtitle>

@evgeni We have just unified the subtitles for all guides to use this form, which demonstrates the contents of the guide. Can you please use it and fine tune the details? Not sure if my suggestion is technically correct, it's an example rather.

Copy link
Member Author

Choose a reason for hiding this comment

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

aye. shortened yours a bit, lemme know what you think

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Needs re-review labels Oct 10, 2023
@Lennonka
Copy link
Contributor

Lennonka commented Oct 10, 2023 •
edited

Also, we need to cherry-pick this. I'm adding cherry-picks to 3.7+.

@evgeni
Copy link
Member Author

evgeni commented Oct 11, 2023

Also, we need to cherry-pick this. I'm adding cherry-picks to 3.7+.

No. Several features used in here are not in 3.7, and some are also not in 3.8. As of now, this is strictly 3.9+

@asteflova
Copy link
Contributor

How about we merge it more-or-less "as is", then I would suggest the further structural improvements in a new PR, and we continue the review then?

+1 Rather than make Evgeni implement all our nerdy doc style improvements, it would be better to have one of the writers take over to do some editing. As long as the PR is okay from a technical perspective, it's perfectly fine to be merged into master/3.9.

@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Oct 11, 2023
@maximiliankolb
Copy link
Contributor

Thanks Evgeni! I agree with Lena/Anet and think it's fair game to merge as is to "master" and make style adjustments later on. ✌️

@evgeni
Copy link
Member Author

evgeni commented Oct 11, 2023

I think I've addressed all comments from Lena and I do agree that merging it as-is and then do style fixes later seems like a good approach.

@evgeni @maximiliankolb
Fixes #36818 - Monitoring Guide
1976061 
Co-authored-by: Maximilian Kolb <kolb@atix.de>
@evgeni
Copy link
Member Author

evgeni commented Oct 11, 2023

(force push was squash only)

@maximiliankolb
Copy link
Contributor

Feel free to merge @Lennonka

@Lennonka Lennonka merged commit 8f111b0 into master Oct 12, 2023
7 of 8 checks passed
@Lennonka Lennonka deleted the monitoring-guide branch October 12, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl left review comments

@ehelms ehelms ehelms left review comments

@pablomh pablomh pablomh left review comments

@asteflova asteflova asteflova left review comments

@mjivraja mjivraja mjivraja left review comments

@Lennonka Lennonka Lennonka requested changes

@maximiliankolb maximiliankolb maximiliankolb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@evgeni @ehelms @mjivraja @Lennonka @asteflova @maximiliankolb @ekohl @pablomh