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

[Python] Add documentation of auto-instrumenting Python services using the operator #2159

Merged

Conversation

TylerHelmuth
Copy link
Member

Adds a new section for Python automatic docs detailing how to use the OpenTelemetry operator to inject auto-instrumentation.

If we like this I can do something similar for the other languages the operator can inject.

@TylerHelmuth TylerHelmuth requested review from a team as code owners January 12, 2023 17:31
@TylerHelmuth TylerHelmuth force-pushed the python-operator-instrumentation branch from 9a726e0 to f3b2191 Compare January 12, 2023 17:57
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Round of copy-edits and some questions. Really happy to see this come together.

@svrnm
Copy link
Member

svrnm commented Jan 16, 2023

thanks @TylerHelmuth

I am wondering if this should live below https://opentelemetry.io/docs/k8s-operator/ ? Where do you think people might expect it?

cc @pavolloffay

@pavolloffay
Copy link
Member

I am wondering if this should live below https://opentelemetry.io/docs/k8s-operator/ ? Where do you think people might expect it?

I would prefer to keep it in the operator docs, but crosslink it from the python instrumentation docs.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Good work,

as mentioned in another comment. A better place might be directly under operator docs.

We should also think how other operator auto-instrumentations will be documented and try to reuse common sections (collector deployment etc.)

@cartermp
Copy link
Contributor

@pavolloffay Yes, agreed. I think we can iterate on this. We can extract various sections as subpages and share (e.g., collector stuff, some commentary) or break some of it up into separate articles (although only if it doesn't complicate the end-user experience).

@TylerHelmuth how would you like to proceed? We can take it in as-is and I can tweak from there, or we can try to make adjustments in this PR. I can PR against your branch if that works.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Hi all. If someone could run the markdown Prettifier over the new file before it is merged, that would be great! Otherwise it can be handled later (esp. since @cartermp is considering iterative improvements).

@TylerHelmuth
Copy link
Member Author

I can update this PR on Tuesday to reorganize the content.

@TylerHelmuth TylerHelmuth requested review from a team as code owners January 17, 2023 22:06
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Some copy-edits. Mainly reducing header sizes, since there's a top-level walkthrough but all the content is in subsections, so we can probably just move those subsections up one level.

content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
TylerHelmuth and others added 13 commits January 17, 2023 15:22
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@TylerHelmuth TylerHelmuth force-pushed the python-operator-instrumentation branch from 78af4e2 to aace230 Compare January 17, 2023 22:23
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

This sparks joy.

@pavolloffay and @open-telemetry/python-approvers can you take a look please?

@TylerHelmuth
Copy link
Member Author

@pavolloffay @svrnm @cartermp @chalin

Moved content under k8s-operator and tried to keep the "walkthrough" feel of the doc. My thought was that the ins and outs of the different operator CRs and other configuration options is best handled in the operator's documentation and this section should be used as a "I want to auto-instrument a language, give me step by step instructions".

content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Show resolved Hide resolved
@svrnm
Copy link
Member

svrnm commented Jan 18, 2023

just as a final remark: k8s idiots like me have no idea where and how to apply those annotations, I think I have googled that now a few times and still can not remember, so as a follow up to this it would be good to have an example for that as well.

TylerHelmuth and others added 2 commits January 18, 2023 09:13
Co-authored-by: Severin Neumann <severin.neumann@altmuehlnet.de>
@TylerHelmuth
Copy link
Member Author

@svrnm each section of yaml is already ready to be copy and pasted into a terminal and use kubectl to apply the resource. Is there more you think we should add?

I purposefully added the ability to copy and paste the sections but didn't want to get into details about kubectl itself or the various methods of applying resource in k8s. My thinking was that if the reader feels comfortable using an operator (which I view as an advanced k8s technique), then they should already know how to apply resources using kubectl.

@svrnm
Copy link
Member

svrnm commented Jan 23, 2023

@svrnm each section of yaml is already ready to be copy and pasted into a terminal and use kubectl to apply the resource. Is there more you think we should add?

I purposefully added the ability to copy and paste the sections but didn't want to get into details about kubectl itself or the various methods of applying resource in k8s. My thinking was that if the reader feels comfortable using an operator (which I view as an advanced k8s technique), then they should already know how to apply resources using kubectl.

I know plenty of people that use the operator without really knowing a lot of basics for k8s ;-) ... but I get your point. Giving one way of many and calling out that there are alternatives would be my preferred solution, but that's optional and from my point of view we should be good to get this into the docs?

@open-telemetry/operator-approvers any concerns?

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thanks @TylerHelmuth

@cartermp
Copy link
Contributor

@open-telemetry/operator-approvers -- any feedback? I'll merge within the week since it's been vetted by several folks at this point

@svrnm svrnm merged commit 13220f3 into open-telemetry:main Jan 24, 2023
@svrnm
Copy link
Member

svrnm commented Jan 24, 2023

🎉

@TylerHelmuth TylerHelmuth deleted the python-operator-instrumentation branch January 24, 2023 17:48
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.

None yet

6 participants