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

ref: Event Type #2753

Merged
merged 70 commits into from
Mar 12, 2024
Merged

ref: Event Type #2753

merged 70 commits into from
Mar 12, 2024

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Feb 21, 2024

Implements type hinting for Event via a TypedDict. This PR mainly adjusts type hints; however, there are also some minor code changes to make the code type-safe following the new changes.

Some items in the Event could have their types expanded by being defined as TypedDicts themselves. These items have been indicated with TODO comments.

Fixes GH-2357

@szokeasaurusrex
Copy link
Member Author

Will need to either:

  • Rebase on 2.0 branch
  • Move to functional TypedDict syntax

@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review March 11, 2024 15:15
@szokeasaurusrex szokeasaurusrex changed the title WIP: Event Type ref: Event Type Mar 11, 2024
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Great work @szokeasaurusrex looks great!
I have a few questions.

sentry_sdk/_types.py Show resolved Hide resolved
sentry_sdk/_types.py Outdated Show resolved Hide resolved
sentry_sdk/client.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/gql.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/logging.py Show resolved Hide resolved
sentry_sdk/integrations/rq.py Show resolved Hide resolved
sentry_sdk/integrations/starlite.py Show resolved Hide resolved
sentry_sdk/integrations/strawberry.py Show resolved Hide resolved
sentry_sdk/integrations/tornado.py Show resolved Hide resolved
sentry_sdk/profiler.py Outdated Show resolved Hide resolved
Copy link
Member Author

@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.

Thank you for the feedback! I replied to your comments and made some changes

sentry_sdk/_types.py Show resolved Hide resolved
sentry_sdk/_types.py Outdated Show resolved Hide resolved
sentry_sdk/_types.py Outdated Show resolved Hide resolved
sentry_sdk/client.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/gql.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/logging.py Show resolved Hide resolved
Comment on lines 66 to 67
with capture_internal_exceptions():
contexts.setdefault("trace", {}).update(sentry_span.get_trace_context()) # type: ignore[attr-defined]
Copy link
Member Author

Choose a reason for hiding this comment

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

Added for same reason as above, and similarly we don't need it any more

sentry_sdk/integrations/tornado.py Show resolved Hide resolved
sentry_sdk/profiler.py Outdated Show resolved Hide resolved
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Typing rocks!

sentry_sdk/_types.py Show resolved Hide resolved
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) March 12, 2024 15:01
@szokeasaurusrex szokeasaurusrex merged commit 5717f1b into master Mar 12, 2024
123 of 124 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/event-type branch March 12, 2024 15:21
szokeasaurusrex added a commit that referenced this pull request Mar 13, 2024
* ref: Improve scrub_dict typing (#2768)

This change improves the typing of the scrub_dict method.

Previously, the scrub_dict method's type hints indicated that only dict[str, Any] was accepted as the parameter. However, the method is actually implemented to accept any object, since it checks the types of the parameters at runtime. Therefore, object is a more appropriate type hint for the parameter.

#2753 depends on this change for mypy to pass

* Propagate sentry-trace and baggage to huey tasks (#2792)

This PR enables passing `sentry-trace` and `baggage` headers to background tasks using the Huey task queue.

This allows easily correlating what happens inside a background task with whatever transaction (e.g. a user request in a Django application) queued the task in the first place.

Periodic tasks do not get these headers, because otherwise each execution of the periodic task would be tied to the same parent trace (the long-running worker process).

--- 

Co-authored-by: Anton Pirker <[email protected]>

* OpenAI integration (#2791)

* OpenAI integration

* Fix linting errors

* Fix CI

* Fix lint

* Fix more CI issues

* Run tests on version pinned OpenAI too

* Fix pydantic issue in test

* Import type in TYPE_CHECKING gate

* PR feedback fixes

* Fix tiktoken test variant

* PII gate the request and response

* Rename set_data tags

* Move doc location

* Add "exclude prompts" flag as optional

* Change prompts to be excluded by default

* Set flag in tests

* Fix tiktoken tox.ini extra dash

* Change strip PII semantics

* More test coverage for PII

* notiktoken

---------

Co-authored-by: Anton Pirker <[email protected]>

* Add a method for normalizing data passed to set_data (#2800)

* Discard open spans after 10 minutes (#2801)

OTel spans that are handled in the Sentry span processor can never be finished/closed. This leads to a memory leak. This change makes sure that open spans will be removed from memory after 10 minutes to prevent memory usage from growing constantly.

Fixes #2722

---------

Co-authored-by: Daniel Szoke <[email protected]>

* ref: Event Type (#2753)

Implements type hinting for Event via a TypedDict. This commit mainly adjusts type hints; however, there are also some minor code changes to make the code type-safe following the new changes.

Some items in the Event could have their types expanded by being defined as TypedDicts themselves. These items have been indicated with TODO comments.

Fixes GH-2357

* Fix mypy in `client.py`

* Fix functools import

* Fix CI config problem

... by running `python scripts/split-tox-gh-actions/split-tox-gh-actions.py`

---------

Co-authored-by: Christian Schneider <[email protected]>
Co-authored-by: Anton Pirker <[email protected]>
Co-authored-by: colin-sentry <[email protected]>
@ods
Copy link

ods commented Mar 20, 2024

Looks like a breaking change.

Is there a way to use functions with Event type arguments without type: ignore or erasing type with Any? And example from our flow that worked before this:

def before_send(event: dict[str, Any], _: dict[str, Any]) -> dict[str, Any]:
    for frame in iter_event_frames(event):
        do_smth(frame)
    return event

Now checking with mypy gives an errors:

error: Argument 1 to "iter_event_frames" has incompatible type "dict[str, Any]"; expected "Event"  [arg-type]

I'd be happy to switch to using Event typed dict, but 1) it's not publicly exposed, 2) it's defined behind if TYPE_CHECKING which is not always acceptable.

@szokeasaurusrex
Copy link
Member Author

Hi @ods, thank you for reaching out!

I'd be happy to switch to using Event typed dict, but 1) it's not publicly exposed, 2) it's defined behind if TYPE_CHECKING which is not always acceptable.

We released 1.43.0 earlier today, which re-exports Event as a public type in sentry_sdk.types (see #2829).

However, because we need to maintain compatibility with old Python versions, we need to define the Event type behind an if TYPE_CHECKING block. You should be able, however, to use the type hint in your code by importing Event from within an if TYPE_CHECKING block, and then enclosing the type in quotes, like so:

from typing import TYPE_CHECKING
if TYPE_CHECKING:
    from sentry_sdk.typing import Event

def before_send(event: "Event", _: dict[str, Any]) -> "Event":
    ...

Looks like a breaking change.

I understand your frustration, and while I certainly believe we made a mistake by using a private type on publicly exposed functions, our reasoning with this change is that it is not breaking because it only affects type hints, which are not evaluated at runtime and not enforced by the Python language. The code is fully backwards-compatible at runtime, and the runtime public API has remained the same as previously – any code written before this change will continue to work the same now after the change.

@ods
Copy link

ods commented Mar 20, 2024

Thanks for quick response!

However, because we need to maintain compatibility with old Python versions, we need to define the Event type behind an if TYPE_CHECKING block.

Strictly speaking it's not the only option. The type can be defined unconditionally, but imported inside if TYPE_CHECKING block. This way it won't break in old Python versions, because module is not imported.

Ok, anyway we have to find some workaround.

@szokeasaurusrex
Copy link
Member Author

Strictly speaking it's not the only option. The type can be defined unconditionally, but imported inside if TYPE_CHECKING block. This way it won't break in old Python versions, because module is not imported.

This is a good idea! I can certainly look into implementing a conditional import that ensures Event is always defined.

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.

Create Event class/type
3 participants