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

feat(python): Improve integrations page #10575

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Jul 3, 2024

DESCRIBE YOUR PR

Bringing the Python integrations page a bit closer to Node's with more examples, config options, etc.

  • add examples for different integrations-related config options to the integrations page
  • fix false -> False capitalization
  • fix some generic wording ("In some SDKs..." -> Python specific behavior)

Follow up in another PR: expand the integrations table with what functionality each integration offers (tracing, errors, etc.)

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

EXTRA RESOURCES

Copy link

vercel bot commented Jul 3, 2024

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

Name Status Preview Comments Updated (UTC)
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 4, 2024 2:49pm

Copy link

codecov bot commented Jul 3, 2024

Bundle Report

Changes will decrease total bundle size by 15 bytes ⬇️

Bundle name Size Change
sentry-docs-server 8.29MB 6 bytes ⬇️
sentry-docs-edge-server 249.71kB 3 bytes ⬇️
sentry-docs-client 7.87MB 6 bytes ⬇️

@sentrivana sentrivana marked this pull request as ready for review July 3, 2024 10:43
@sentrivana sentrivana requested a review from a team July 3, 2024 10:44
docs/platforms/python/configuration/options.mdx Outdated Show resolved Hide resolved
docs/platforms/python/integrations/index.mdx Outdated Show resolved Hide resolved
docs/platforms/python/integrations/index.mdx Outdated Show resolved Hide resolved
docs/platforms/python/integrations/index.mdx Outdated Show resolved Hide resolved
docs/platforms/python/integrations/index.mdx Outdated Show resolved Hide resolved
docs/platforms/python/integrations/index.mdx Outdated Show resolved Hide resolved
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.

Generally looks good, although please consider my suggestions

@@ -193,7 +193,7 @@ A path to an alternative CA bundle file in PEM-format.

<ConfigKey name="send-client-reports">

Set this boolean to `false` to disable sending of client reports. Client reports are a protocol feature that let clients send status reports about themselves to Sentry. They are currently mainly used to emit outcomes for events that were never sent.
Set this boolean to `False` to disable sending of client reports. Client reports are a protocol feature that let clients send status reports about themselves to Sentry. They are currently mainly used to emit outcomes for events that were never sent.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Set this boolean to `False` to disable sending of client reports. Client reports are a protocol feature that let clients send status reports about themselves to Sentry. They are currently mainly used to emit outcomes for events that were never sent.
Set this boolean to `False` to disable sending client reports. Client reports include data about events that were dropped before being sent to Sentry, as well as other status information about the client.

Maybe also elaborate on what other status information we send, or just drop this if we only are recording missed data for now (perhaps mentioning that we could add additional data here in the future)?

@@ -203,15 +203,15 @@ For many platform SDKs integrations can be configured alongside it. On some plat

<ConfigKey name="integrations" />

In some SDKs, the integrations are configured through this parameter on library initialization. For more information, please see our documentation for a specific integration.
Fine-tune the integrations to enable. See [the integrations page](/platforms/python/integrations) for examples.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Fine-tune the integrations to enable. See [the integrations page](/platforms/python/integrations) for examples.
List of integrations to enable. See [the integrations page](/platforms/python/integrations) for examples.

"Fine-tune" is a bit vague, let's be more specific about what the parameter does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

List of integrations to enable is a bit misleading though. It could be understood to imply that this overrides the logic that auto-enables integrations. Maybe:

List of integrations to enable in addition to auto-enabling integrations (link to integrations page). This setting can be used to override the default config options for a specific auto-enabling integration or to add an integration that is not auto-enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like your suggestion!


<ConfigKey name="default-integrations" />

This can be used to disable integrations that are added by default. When set to `false`, no default integrations are added.
This can be used to disable integrations that are added by default. When set to `False`, no default integrations are added.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This can be used to disable integrations that are added by default. When set to `False`, no default integrations are added.
Configures whether [default integrations](https://docs.sentry.io/platforms/python/integrations/default-integrations/) should be enabled. The default is `True`.
Setting `default_integrations` to `False` disables all default integrations **as well as all auto-enabling integrations**, unless they are specifically added in the `integrations` option, described above.

I would also flip the order of the default-integrations and auto-enabling-integrations section

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 make a separate PR for this if you feel it is out of scope here


<ConfigKey name="auto-enabling-integrations" />

This can be used to disable integrations that are enabled by default if the SDK detects that the corresponding framework or library is installed. When set to `false`, none of these integrations will be enabled by default, even if the corresponding framework/library is detected.
This can be used to disable integrations that are enabled by default if the SDK detects that the corresponding framework or library is installed. When set to `False`, none of these integrations will be enabled by default, even if the corresponding framework/library is detected.
Copy link
Member

Choose a reason for hiding this comment

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

Let's rewrite this similarly to my suggestion for default_integrations

- [List of Default Integrations](default-integrations/)
### Default Integrations

- [List of default integrations](default-integrations/)
Copy link
Member

Choose a reason for hiding this comment

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

I find it somewhat strange that we have a table for all the other integration categories, except this one. Perhaps we can add a table, and then provide the link to the default_integrations page for anyone desiring more information about a specific integration (or even better perhaps, we could have each row in the table link to the relevant subsection in the Default Integrations page)?


### Enabling Integrations

Integrations can be added using the `integrations` config option.
Copy link
Member

Choose a reason for hiding this comment

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

Let's link to the page where the integrations config option is defined here


Integrations can be added using the `integrations` config option.

Integrations marked as "auto-enabled" in the above table will be turned on automatically with default configuration. If you want to fine-tune a specific integration's settings (for instance, change Flask's default `transaction_style`), add it to your `integrations` list as you would a non-auto-enabling integration and pass in the desired options.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Integrations marked as "auto-enabled" in the above table will be turned on automatically with default configuration. If you want to fine-tune a specific integration's settings (for instance, change Flask's default `transaction_style`), add it to your `integrations` list as you would a non-auto-enabling integration and pass in the desired options.
Integrations marked as "auto-enabled" in the above table will be turned on automatically, unless you set `auto-enabling-integrations` to `False`. If you want to configure a specific integration's settings (for instance, change Flask's default `transaction_style`), add it to your `integrations` list as you would a non-auto-enabling integration and pass in the desired options.

Also link to the auto_enabling_integrations option's docs page if possible

Comment on lines +147 to +152
* **Default integrations** like `logging` or `excepthook` are always enabled,
regardless of what packages you have installed. They provide essential
SDK functionality like error deduplication or event flushing at interpreter
shutdown.
* **Auto-enabling integrations** like `FlaskIntegration` are automatically added
if the SDK detects you have the corresponding package (like Flask) installed.
Copy link
Member

Choose a reason for hiding this comment

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

It would make sense to mention here that these integrations are only enabled when the respective option is set to True

Comment on lines +154 to +157
If you want to disable a specific auto-enabling integration, first turn off all
auto-enabling integrations by setting `auto_enabling_integrations` to `False`.
Any originally auto-enabled integrations that you want to use then have to be
explicitly added via the `integrations` config option.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you want to disable a specific auto-enabling integration, first turn off all
auto-enabling integrations by setting `auto_enabling_integrations` to `False`.
Any originally auto-enabled integrations that you want to use then have to be
explicitly added via the `integrations` config option.
To disable a specific default or auto-enabling integration, first disable all auto-enabling integrations by setting the `auto_enabling_integrations` option to `False`. Then, explicitly enable all the auto-enabling integrations you wish to use by setting the `integrations` configuration option.

)
```

In case you want to disable one of the [default integrations](default-integrations/),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In case you want to disable one of the [default integrations](default-integrations/),
To disable one of the [default integrations](default-integrations/),

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

2 participants