-
Notifications
You must be signed in to change notification settings - Fork 820
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.
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
Update metrics documentation for Cloud Monitoring/Stackdriver #2862
Conversation
- Update Stackdriver references to Cloud Monitoring - Include missing steps to get Cloud Monitoring up and running - Add step to resolution for the error mentioned in the troubleshooting section
/assign @markmandel |
Build Failed 😱 Build Id: 041ec12d-a796-466e-84ff-f1a9f167e489 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Looks like your docs change have introduced a broken link unfortunately:
|
Looks like an error was caught by htmltest:
|
Build Failed 😱 Build Id: b01701d8-6e16-4c24-8476-27b4ae60b9bf To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Fix broken link Update additional stackdriver references
Build Succeeded 👏 Build Id: 44304477-01a7-4fbd-ac87-93c669c84956 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Co-authored-by: aimuz <[email protected]>
Build Succeeded 👏 Build Id: 7262a8df-6b0e-4ccd-a4cd-01e4e884a0f7 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
LGTM Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing this work! This is great. I am worried that section on outputting a yaml file could be confusing though - have a look at my comment and let me know what you think.
If you are using the [YAML installation]({{< ref "/docs/Installation/Install Agones/yaml.md" >}}), you will need to update the necessary parameters in the `install.yaml` file using the following commands: | ||
|
||
#### Using Stackdriver with Workload Identity | ||
```bash | ||
helm pull --untar https://agones.dev/chart/stable/agones-1.27.0.tgz && \ | ||
cd agones && \ | ||
helm template agones-manual --namespace agones-system . \ | ||
--set agones.metrics.stackdriverEnabled=true \ | ||
--set agones.metrics.prometheusEnabled=false \ | ||
--set agones.metrics.prometheusServiceDiscovery=false \ | ||
> install-custom.yaml | ||
``` | ||
|
||
If you would like to enable stackdriver in conjunction with [Workload Identity](https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity), there are a few extra steps you need to follow: | ||
```bash | ||
kubectl apply -f install-custom.yaml | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I'm not sure about this section. I'm kinda figuring it's not necessary and possibly confusing.
If you are using the YAML install, running this command doesn't actually change the install.yaml that is hosted on github for the release - it just gives you an yaml file.
If someone is familiar with Helm and wants a yaml file instead, I would argue it's probably likely they already know how to do this.
If they aren't familiar with Helm, I'd worry this section is confusing. I'd be tempted to drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I definitely could have phrased that more accurately. Similarly to the Install Agones using YAML page, I do think having some guidance around what to do if you are using YAML would be helpful. An alternative would be to simply add a note linking to this part of the page to change the parameters.
I'm also not opposed to dropping it but let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't see a lot of people finding confusion with generating yaml from Helm -- but it's entirely possible that's a blind spot I have.
An alternative would be to simply add a note linking to this part of the page to change the parameters.
Seems like a good compromise to me 👍🏻 Could even do it as an "Note" box if you want to highlight it, e.g.
agones/site/content/en/docs/Guides/metrics.md
Lines 114 to 116 in 1cc9a3b
{{< alert title="Note" color="info">}} | |
You can import our dashboards by copying the json content from {{< ghlink href="/build/grafana" branch="main" >}}each config map{{< /ghlink >}} into your own instance of Grafana (+ > Create > Import > Or paste json) or follow the [installation](#installation) guide. | |
{{< /alert >}} |
Build Failed 😱 Build Id: 97a93300-0faa-45cb-9535-678a4b6718f3 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Change format to be a note referring to the instructions on the installation page
Build Succeeded 👏 Build Id: 3f6736b6-c9ce-4292-998d-64280c2256d9 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realised I never came back and approved this. It's good to go!
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: junninho, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: junninho, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build Succeeded 👏 Build Id: 7a651389-1ecb-491e-9fca-b65f04f29a09 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind documentation
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #2850
Special notes for your reviewer: