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

Add readTheDocs #252

Merged
merged 7 commits into from
Jan 26, 2021
Merged

Add readTheDocs #252

merged 7 commits into from
Jan 26, 2021

Conversation

NathanielRN
Copy link
Contributor

Description

Adds the readTheDocs build to the Contrib repo. I went back to the Core PR open-telemetry/opentelemetry-python#1258 where I removed the documentation for the individual instrumentation packages and added them back now in this PR.

However it ended up being much more annoying than I thought :)

I kept getting errors such as

py:class reference target not found: opentelemetry.instrumentation.instrumentor.BaseInstrumentor

Even though the test correctly installed all 3 of the API, SDK, and Instrumentation repos from GitHub correctly through VCS pip installs. (I verified that if I deleted the relevant .tox/docs/src/opentelemetry-instrumentation/opentelemetry-instrumentation/ folder the Docs output a helpful message "error this module does not exist!")

But I finally found this StackOverflow post which explained that this is a known issue with Sphinx.

I think the problem is that Sphinx will output warnings when imports are from OTHER REPOS, even though it can verify that they do exist in the Python install path.

This solution goes with the solution suggested in the StackOverflow post, but with an easy to comment/uncomment nitpick-exceptions.ini file.

I think it's worth it to keep the "error on warning" setting on because it will show us if our docs have invalid imports (although maybe that's that's what unit tests are supposed to be for???)

Fixes #194

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Both tox -e docs and make html produce html files in _bulid/html that make sense!

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@NathanielRN NathanielRN requested review from a team, codeboten and lzchen and removed request for a team December 12, 2020 05:44
@NathanielRN NathanielRN force-pushed the add-docs branch 3 times, most recently from 9e60eaa to a9c9bbc Compare December 12, 2020 07:34
Comment on lines +116 to +128
if "class_references" in mcfg:
class_references = getlistcfg(mcfg["class_references"])
for class_reference in class_references:
nitpick_ignore.append(("py:class", class_reference,))

if "anys" in mcfg:
anys = getlistcfg(mcfg["anys"])
for any in anys:
nitpick_ignore.append(("any", any,))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to highlight this as my implementation of the solution described in the Stack Overflow post as mentioned in the PR description.

Copy link
Member

Choose a reason for hiding this comment

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

Will this break the ref links? For example in the current stable django documentation https://opentelemetry-python.readthedocs.io/en/stable/instrumentation/django/django.html If you click on Bases: [opentelemetry.instrumentation.instrumentor.BaseInstrumentor] or See [BaseInstrumentor] They both take you BaseInstrumentor documentation. If I understand it correctly as these instrumentation libraries are moved out of main repo it can't find such references anymore and giving error. They are now cross-referencing targets from main repo docs.

will intersphinx_mapping along with updating `BaseInstrumentor` occurrences with `opentelemetry.instrumentation.instrumentor.BaseInstrumentor` be helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I believe you are correct that all the references in this nitpick-exceptions.ini file will produce links that are broken. It's because these components are Core code components and Contrib doesn't know where to link the reference to.

The intersphinx_mapping solution sounds interesting! However it's not something I've explored 😓 although we can create a follow up to fix these links? If we want to keep this PR small we can merge it like this, and then we can create an issue to investigate that mapping solution to link to the Core docs?

The main benefit this PR does is that at least we can have auto documentation on the packages even though the links to the Core components are broken.

Also, the docs will break if the SDK components break/change. But as I mentioned in the PR description, maybe that shouldn't be the job of the Docs, maybe that should be the job of the unit tests?

Copy link
Member

Choose a reason for hiding this comment

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

we can create an issue to investigate that mapping solution to link to the Core docs?

This would be ideal. I just checked, all the references in this nitpick-exceptions.ini will not produce the broken links rather it is just a plain text instead of linked documentation. The intersphinx_mapping should really help us here for example pymongo instrumentation documentation has an link to official pymongo driver documentation here

Copy link
Member

Choose a reason for hiding this comment

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

Think you should be able to remove all of this with the intersphinx mapping?

),
"aiohttp": ("https://aiohttp.readthedocs.io/en/stable/", None),
"wrapt": ("https://wrapt.readthedocs.io/en/latest/", None),
"pymongo": ("https://pymongo.readthedocs.io/en/stable/", None),
Copy link
Member

Choose a reason for hiding this comment

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

I have an OTel exporter in this repo that the doc build works correctly https://github.com/GoogleCloudPlatform/opentelemetry-operations-python/blob/c3772de4cf50950f1d51685ff3bbf0e46ee026df/docs/conf.py#L50-L56

Suggested change
"pymongo": ("https://pymongo.readthedocs.io/en/stable/", None),
"pymongo": ("https://pymongo.readthedocs.io/en/stable/", None),
"opentelemetry": (
"https://opentelemetry-python.readthedocs.io/en/latest/",
None,
),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So adding this intersphinx-mapping almost completely solved the problem! However there's still a few items that need to be ignore by the nitpick...

In nitpick-exceptions.ini I still have to silence the errors from these:

[default]
class_references=
    ; TODO: Understand why sphinx is not able to find this local class
    opentelemetry.trace.propagation.textmap.TextMapPropagator
    ; - AwsXRayFormat
    opentelemetry.trace.propagation.textmap.DictGetter
    ; - instrumentation.asgi.CarrierGetter
    ; API
    opentelemetry.trace.propagation.textmap.Getter
    ; - DatadogFormat
    ; - AWSXRayFormat
    TextMapPropagatorT
    ; - AwsXRayFormat.extract

anys=
    ; API
    opentelemetry.trace.propagation.textmap.TextMapPropagator.fields
    ; - AWSXRayFormat
    TraceId
    ; - AwsXRayIdsGenerator
    TraceIdRatioBased
    ; - AwsXRayIdsGenerator
    ; SDK
    SpanProcessor
    ; - DatadogExportSpanProcessor
    TracerProvider
    ; - AwsXRayIdsGenerator
    ; Instrumentation
    BaseInstrumentor
    ; - instrumentation.*

I added comments showing where the errors come from. This makes me wonder if I did the doc string for AWS components here wrong? Is there a certain way that I need to follow for the backticks `` when writing doc strings?

Something that would make the following doc string break such that I needed the nitpick-exceptions above?

AWS X-Ray IDs Generator
-----------------------
The **AWS X-Ray IDs Generator** provides a custom IDs Generator to make
traces generated using the OpenTelemetry SDKs `TracerProvider` compatible
with the AWS X-Ray backend service `trace ID format`_.

Copy link
Member

Choose a reason for hiding this comment

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

I dont think the docstring should matter. Automodule should generate entries regardless of docstring, as long as you have the .. automodule:: opentelemetry....

Do you see the AWS docs built locally?

@aabmass
Copy link
Member

aabmass commented Dec 17, 2020

But I finally found this StackOverflow post which explained that this is a known issue with Sphinx.

I think the problem is that Sphinx will output warnings when imports are from OTHER REPOS, even though it can verify that they do exist in the Python install path.

This solution goes with the solution suggested in the StackOverflow post, but with an easy to comment/uncomment nitpick-exceptions.ini file.

The linked SO post seems to only be talking about builtins. I think the other py:class reference target not found are either related to intersphinx mapping being missing or the packages are not installed. I have a sphinx docs build working fine here that references OTel packages and I didn't need this ini stuff

docs/conf.py Show resolved Hide resolved
@@ -0,0 +1,7 @@
OpenTelemetry WSGI Instrumentation
==================================

Copy link
Member

Choose a reason for hiding this comment

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

Some other instrumentation such as tornado and urllib etc.. are missing, any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I noticed that earlier too, I just pulled the files that were on Core so I assume they hadn’t been updated on Core either.

For that reason I thought as a follow up we can add those docs.

@codeboten codeboten added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 26, 2021
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.

Approving this to get the read the docs started for this repo, we can always improve them as we go.

docs/instrumentation/asyncpg/asyncpg.rst Outdated Show resolved Hide resolved
@codeboten codeboten merged commit c9075cf into open-telemetry:master Jan 26, 2021
@NathanielRN NathanielRN deleted the add-docs branch June 29, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add read the docs build to Contrib repo
4 participants