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

Update README.md #1428

Merged
merged 8 commits into from
Dec 8, 2020
Merged

Update README.md #1428

merged 8 commits into from
Dec 8, 2020

Conversation

lizsnyder
Copy link
Contributor

Description

General readability improvements, grammatical fixes, and general conformity to google developer documentation style guide.

Checklist:

  • [X ] Followed the style guidelines of this project
  • [ X] Changelogs have been updated
  • [ X] Unit tests have been added
  • [X ] Documentation has been updated

@lizsnyder lizsnyder requested review from a team, owais and hectorhdzg and removed request for a team November 27, 2020 01:41
@lizsnyder
Copy link
Contributor Author

lizsnyder commented Nov 27, 2020

@owais I was a little thrown off by the following paragraph:

"Libraries that produce telemetry data should only depend on opentelemetry-api, and defer the choice of the SDK to the application developer. Applications may depend on opentelemetry-sdk or another package that implements the API."

The use of "should" and "may" are ambiguous here and should be replaced with more direct language. Are we telling users that the libraries only depend on opentelemetry-api? Or is this somehow a suggestion to the user?

Similarly, are we saying applications might depend on the SDK (in this case, when would this happen?), or are we describing something the user's application is allowed to do?

Any help you can give on having more direct language is much appreciated.

@codeboten
Copy link
Contributor

I just noticed that the README doesn't include any information pointing users at the Python contrib repository for additional exporter and instrumentation packages. Do you think it's worth adding a note around this @snyder114?

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

LGTM

Added reference to opentelemetry-python-contrib repo.
@lizsnyder lizsnyder closed this Dec 4, 2020
@lizsnyder
Copy link
Contributor Author

I just noticed that the README doesn't include any information pointing users at the Python contrib repository for additional exporter and instrumentation packages. Do you think it's worth adding a note around this @snyder114?

@codeboten good call, I added a reference. Do you have any input on my first question above? If not, do you mind merging this PR? No write access

@lizsnyder lizsnyder reopened this Dec 4, 2020
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

@lizsnyder
Copy link
Contributor Author

@codeboten thanks for the resource. Looks like I found some more docs to work on :) I fixed the text and I think it conveys the correct message. Could you merge when you have a minute?

README.md Outdated
The `opentelemetry-sdk` package is the reference implementation of the API.

Libraries that produce telemetry data should only depend on `opentelemetry-api`,
and defer the choice of the SDK to the application developer. Applications may
Libraries that produce telemetry data only need to depend on `opentelemetry-api`,
Copy link
Contributor

@lzchen lzchen Dec 7, 2020

Choose a reason for hiding this comment

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

I think the purpose of "should only" is to outline that the sdk should not be taken a dependency on by these libraries. I think the rewording "only needs to" loosens this definition.

Copy link
Contributor

@lzchen lzchen Dec 7, 2020

Choose a reason for hiding this comment

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

The use of "should" and "may" are ambiguous here and should be replaced with more direct language. Are we telling users that the libraries only depend on opentelemetry-api? Or is this somehow a suggestion to the user?

As @codeboten outlined, we are following the specifications for components and behaviour that need to follow "should", "may" and "must" descriptions. They are currently documented to the best of our abilities, but we do not want to change or loosen the definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I understand, thanks. I replaced with the original terminology.

@lizsnyder
Copy link
Contributor Author

@lzchen after you merged develop it looks like a dependency is missing. Could you help me fix the errors? A little beyond my knowledge.

@lzchen lzchen merged commit e6c2f44 into open-telemetry:master Dec 8, 2020
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Dec 8, 2020
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.

4 participants