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.

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

Clear up language and remove duplicate content #1202

Merged
merged 3 commits into from
Apr 30, 2019
Merged

Clear up language and remove duplicate content #1202

merged 3 commits into from
Apr 30, 2019

Conversation

scottilee
Copy link
Contributor

What is the problem I am trying to address?

Describe the issue you have been trying to solve.

There were some sentences in the "Is there support for monitoring and observability for Proxy?" FAQ that were written too informally.

Additionally, documentation for installing Athens on Kubernetes had duplicate directions.

Screen Shot 2019-04-29 at 6 16 24 PM

How is the fix applied?

Mention briefly how you have applied the fix.

I re-worded the sentences and removed the duplicate lines.

Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.

Fixes #

There is no associated issue with this fix/pull request.

@scottilee scottilee requested a review from a team as a code owner April 30, 2019 01:17
Copy link
Member

@arschles arschles left a comment

Choose a reason for hiding this comment

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

@scottilee sorry for the delay here, but this looks great 🚀 :shipit:. I don't want to delay this PR on my question on the helm install command, so I'm going to merge. Can you respond in the thread after the PR is merged, or tell me in slack?

```console
helm install gomods/athens-proxy -n athens --namespace athens --set replicaCount=3
```

Copy link
Member

Choose a reason for hiding this comment

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

@scottilee I should know this by now, but is this command somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the screenshot I posted they're right above and below each other. Repasting the screenshot:
Screen Shot 2019-04-29 at 6 16 24 PM

I figured it was an "advanced" command so I left it in that section instead of the previous one.

@@ -74,5 +74,4 @@ To try out tracing with Jaeger, do the following:
- Run the walkthrough tutorial
- Open `http://localhost:16686/search`

Observability is not a hard requirement for the Athens proxy. So, if the infrastructure is not properly set up, they will fail with an information log. For eg. if Jaeger is not running / if the wrong URL to the exporter is provided, proxy will continue to run. However, it will not collect any traces or metrics (when the exporter backend is unavailable).

Observability is not a hard requirement for the Athens proxy. So, if the infrastructure is not properly set up, it will fail with an information log. For example, if Jaeger is not running or if the wrong URL to the exporter is provided, the proxy will continue to run. However, it will not collect any traces or metrics while the exporter backend is unavailable.
Copy link
Member

Choose a reason for hiding this comment

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

❤️ this!

@arschles arschles merged commit a327db0 into gomods:master Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants