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] Overhaul "Setup tracing" > "Custom instrumentation" #11341

Merged
merged 6 commits into from
Oct 2, 2024

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Sep 12, 2024

DESCRIBE YOUR PR

  • Add documentation for manual span creation (without context manager)
  • Clean up code snippets
  • Made all callout boxes the same color, to make page more calm.

Preview Link:
https://sentry-docs-git-antonpirker-pythoncustom-tracing-information.sentry.dev/platforms/python/tracing/instrumentation/custom-instrumentation

IS YOUR CHANGE URGENT?

Help us prioritize incoming PRs by letting us know when the change needs to go live.

  • Urgent deadline (GA date, etc.):
  • Other deadline:
  • None: Not urgent, can wait up to 1 week+

SLA

  • Teamwork makes the dream work, so please add a reviewer to your PRs.
  • Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it.
    Thanks in advance for your help!

PRE-MERGE CHECKLIST

Make sure you've checked the following before merging your changes:

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs (subject matter experts)
  • PR was reviewed and approved by a member of the Sentry docs team

LEGAL BOILERPLATE

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

EXTRA RESOURCES

Copy link

vercel bot commented Sep 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
develop-docs ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 8:52am
sentry-docs ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 8:52am
changelog ⬜️ Skipped (Inspect) Oct 2, 2024 8:52am

Copy link

codecov bot commented Sep 12, 2024

Bundle Report

Changes will decrease total bundle size by 15 bytes (-0.0%) ⬇️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
sentry-docs-server-cjs 7.43MB 6 bytes ⬇️
sentry-docs-edge-server-array-push 257.01kB 3 bytes ⬇️
sentry-docs-client-array-push 6.4MB 6 bytes ⬇️

Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Nice! Left 2 comments

@antonpirker antonpirker enabled auto-merge (squash) September 12, 2024 08:06
@antonpirker antonpirker changed the title [Python] Over "Setup tracing" > "Custom instrumentation" [Python] Overhaul "Setup tracing" > "Custom instrumentation" Sep 12, 2024
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

My general feedback here is that I think this page is too long and too detailed about too many different concepts. We should delete things or move them to their own pages so they are available for advanced users, without confusing beginner users

def eat_pizza(pizza):
with sentry_sdk.start_transaction(op="task", name="Eat Pizza"):
while pizza.slices > 0:
span = sentry_sdk.start_span(description="Eat Slice")
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to call __enter__ and __exit__ here? We set some things on the scope in those magic methods, is that important?

```

When you create your span manually, make sure to call `span.finish()` after the block of code you want to wrap in a span to finish the span. If you do not finish the span it will not be sent to Sentry.

## Nested Spans

Spans can be nested to form a span tree. If you'd like to learn more, read our [distributed tracing](/product/sentry-basics/tracing/distributed-tracing/) documentation.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the examples in this section are needed, we could probably delete them

Comment on lines +176 to 180
<Alert title="Important" level="info">

To enable performance monitoring for the functions specified in `functions_to_trace`, the SDK needs to load the function modules. Be aware, there may be code being executed in modules during module loading. To avoid this, use the method described above to trace your functions.

</Alert>
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this page already has so much information. This block will just confuse people. We should either remove it, or if it is important, let's describe functions_to_trace in depth on a different page.

Suggested change
<Alert title="Important" level="info">
To enable performance monitoring for the functions specified in `functions_to_trace`, the SDK needs to load the function modules. Be aware, there may be code being executed in modules during module loading. To avoid this, use the method described above to trace your functions.
</Alert>

@@ -145,21 +173,21 @@ sentry_sdk.init(

Now, whenever a function specified in `functions_to_trace` will be executed, a span will be created and attached as a child to the currently running span.

<Alert level="warning" title="Important">
<Alert title="Important" level="info">

To enable performance monitoring for the functions specified in `functions_to_trace`, the SDK needs to load the function modules. Be aware, there may be code being executed in modules during module loading. To avoid this, use the method described above to trace your functions.

</Alert>

## Accessing the Current Transaction
Copy link
Member

Choose a reason for hiding this comment

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

I think we could probably also delete this section or move it to its own page. This is probably too much information for most use cases, especially because users who are doing custom instrumentation should already have a reference to the current transaction, since start_transaction returns the transaction.

@@ -170,7 +198,7 @@ def eat_pizza(pizza):

## Accessing the Current Span
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this section is again possibly too detailed for this page.

@szokeasaurusrex
Copy link
Member

Although, to be fair, this PR is probably already an improvement over what we had before, so I have no objections to merging this change and creating a new issue to address further simplifying this page

@antonpirker
Copy link
Member Author

Thanks for the review @szokeasaurusrex

As for the page being long, I think this is not a big problem. No one reads docs pages from top to bottom. People coming here have a problem and an intend to fix it. They google "python sentry nested spans", land on that page, skim through the page (or CTRL+F) until the find the "Nested Spans" section, figure out how it is done and then leave.

What you propose is in deed a bigger refactoring of the docs and outside of the scope of this PR.
Feel free to create an issue in this repo on how to organize the content differently.

@antonpirker antonpirker self-assigned this Sep 13, 2024
@antonpirker antonpirker enabled auto-merge (squash) October 2, 2024 08:30
@vercel vercel bot temporarily deployed to Preview – changelog October 2, 2024 08:52 Inactive
@antonpirker antonpirker merged commit cc2c50f into master Oct 2, 2024
9 checks passed
@antonpirker antonpirker deleted the antonpirker/python/custom-tracing-information branch October 2, 2024 08:53
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants