-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Cleaning up Python documentation typos #3837
Cleaning up Python documentation typos #3837
Conversation
Updated Python getting-started.md and instrumentation.md pages to fix a mix of typos, grammatical errors. Also re-wrote a couple sentences for clarity.
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 @astanley-work, I provieded a few comments
@@ -21,7 +21,7 @@ Ensure that you have the following installed locally: | |||
## Example Application | |||
|
|||
The following example uses a basic [Flask](https://flask.palletsprojects.com/) | |||
application. If you are not using Flask, that's OK — you can use OpenTelemetry | |||
application. If you have not used Flask before or prefer a different framework, that's OK — you can use OpenTelemetry |
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 think this changes the meaning of this sentence. It was intended to mean "if you are not a flask user (bit django, etc.) you still can follow this tutorial, or you can follow along with your preferred framework, we have it for you", this changes it "if you do not have experience with flask, that's no problem, you still can follow this tutorial, ..."
I think both meanings are justified, maybe we can tune it accordingly.
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 Severin,
I see what you mean. If you would like to adjust it to be closer to the first meaning that is fine with me.
One way to potentially thread both might be "If you are not a Flask user, that's OK - you can still use OpenTelemetry and you can still follow this tutorial."
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.
yeah, that might be better. @open-telemetry/docs-approvers any thoughts on this?
- Creating a single telemetry sink shared by multiple services, to reduce overhead of switching exporters | ||
- Aggregating traces across multiple services, running on multiple hosts | ||
- A central place to process traces prior to exporting them to a backend | ||
- Creating a central place to process traces prior to exporting them to a backend |
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.
the items of that list are related to "when it's beneficial to use a collector", so I think this should be something like "if you need...", not entirely sure, cc @open-telemetry/docs-approvers
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.
Ahh I see what you are saying.
In that case, how does the following phrasing sound?
"Some examples of when it's beneficial to use a collector include when you need to:
- Create a single telemetry sink shared by multiple services, to reduce overhead of switching exporters
- Aggregate traces across multiple services, running on multiple hosts
- Process your traces in a central place prior to exporting them to a backend"
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.
yes, that's better, so works for me 👍
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.
@astanley-work can you apply those changes?
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 a small nit.
application. If you have not used Flask before or prefer a different framework, | ||
you can use OpenTelemetry | ||
Python with other web frameworks as well, such as Django and FastAPI. For a | ||
complete list of libraries for supported frameworks, see the |
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.
What is suggested here seems fine, but it would be nice if we could have some consistency across languages. I proposed something in https://github.com/open-telemetry/opentelemetry.io/pull/3877/files, which adapted here, might read like:
application. If you have not used Flask before or prefer a different framework, | |
you can use OpenTelemetry | |
Python with other web frameworks as well, such as Django and FastAPI. For a | |
complete list of libraries for supported frameworks, see the | |
application. You can use another web framework, such as Django and FastAPI. | |
For a complete list of libraries and supported frameworks, see the |
WDYT @svrnm et al.?
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.
Works for me, also let's turn this in a short code soon
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.
@astanley-work please take a look, click on "commit suggestion" to directly accept @chalin's suggested changes
@astanley-work please tak another look at the open suggestions for your PR + the merge conflict! |
@astanley-work do you want to continue with this PR, if so please take a look at the outstanding issues and the merge conflict, if not please close it |
Updated Python getting-started.md and instrumentation.md pages to fix a mix of typos, grammatical errors. Also re-wrote a couple sentences for clarity.