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

Make it explicit to install distro to get auto-instrumentation working #588

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv requested review from a team, owais and aabmass and removed request for a team July 15, 2021 17:33
@srikanthccv srikanthccv added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 15, 2021
@@ -11,11 +11,14 @@ Installation

::

pip install opentelemetry-instrumentation
pip install opentelemetry-distro[otlp] opentelemetry-instrumentation
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should separate out these sections. Right underneath this, an explanation like what you have in the note would be good (users need to install "A" distro), and then have the pip install follow that excerpt. An explanation that opentelemetry-distro contains the default distro would be good too. This way users won't be confused as to why or what the opentelemetry-distro packge is.

Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

This looks fine but I still think our default recommendation should be to just install the distro and run opentelemetry-instrument as part of getting started docs. It should be followed up with an advanced section that documents other packages and how one may use them without the distro. IMO opentelemetry-distro should be the only or the first package users install when getting started.

@srikanthccv
Copy link
Member Author

Completely agree. That will make a huge difference in terms of user experience for the first the time users. Meanwhile, I wanted to add this note since it is not obvious to someone that without a distro auto instrumentation doesn't produce any telemetry.

@lzchen
Copy link
Contributor

lzchen commented Jul 16, 2021

@ocelotl
Quick ping to make sure this pr doesn't get overlooked during the migration of opentelemetry-instrumentation to core if merged.

@lonewolf3739
Can you fix the builds so we can merge this asap?

open-telemetry/opentelemetry-python#1959

@ocelotl
Copy link
Contributor

ocelotl commented Jul 16, 2021

@ocelotl
Quick ping to make sure this pr doesn't get overlooked during the migration of opentelemetry-instrumentation to core if merged.

@lonewolf3739
Can you fix the builds so we can merge this asap?

open-telemetry/opentelemetry-python#1959

Roger that, @lzchen.

@srikanthccv
Copy link
Member Author

^ @lzchen it's green now.

@lzchen lzchen merged commit 08daa9d into open-telemetry:main Jul 16, 2021
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.

Make it very explicit that you need SOME distro installed.
4 participants