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.

Sign up for GitHub

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

Jump to bottom

[typing/static] telemetry #12714

Merged
merged 2 commits into from
Mar 12, 2023
Merged

[typing/static] telemetry #12714

merged 2 commits into from
Mar 12, 2023

Conversation

smackesey
Copy link
Collaborator

Summary & Motivation

Pure typing PR for telemetry code.

How I Tested These Changes

BK

@vercel
Copy link

vercel bot commented Mar 4, 2023 •
edited
Loading

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

2 Ignored Deployments
Name Status Preview Comments Updated
dagit-storybook ⬜️ Ignored (Inspect) Mar 10, 2023 at 7:32PM (UTC)
dagster ⬜️ Ignored (Inspect) Mar 10, 2023 at 7:32PM (UTC)

This was referenced Mar 4, 2023
Merged
Merged
@smackesey
Copy link
Collaborator Author

smackesey commented Mar 4, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

This was referenced Mar 4, 2023
Merged
Merged
Merged
Merged
Merged
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from b71064c to 6bc42eb Compare March 4, 2023 16:19
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 57bf264 to 697b3b1 Compare March 4, 2023 16:19
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 6bc42eb to a692296 Compare March 4, 2023 19:18
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 697b3b1 to 7156bd7 Compare March 4, 2023 19:18
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from a692296 to a7f8aef Compare March 5, 2023 22:49
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 7156bd7 to cda856f Compare March 5, 2023 22:49
This was referenced Mar 5, 2023
Merged
Merged
Merged
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch 2 times, most recently from d998732 to 5b8fb19 Compare March 6, 2023 01:17
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from cda856f to e450391 Compare March 6, 2023 01:17
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 5b8fb19 to 1863493 Compare March 6, 2023 02:18
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from e450391 to 1522a42 Compare March 6, 2023 02:18
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 1863493 to 6943b29 Compare March 6, 2023 13:30
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 1522a42 to f55f319 Compare March 6, 2023 13:30
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 6943b29 to ce116aa Compare March 6, 2023 15:34
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from f55f319 to 9ba1fe6 Compare March 6, 2023 15:34
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from ce116aa to 74643fd Compare March 6, 2023 20:36
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 9ba1fe6 to bc39291 Compare March 6, 2023 20:37
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 74643fd to 62501a3 Compare March 7, 2023 01:15
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 69ee389 to f4f2d4c Compare March 8, 2023 21:13
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from b99ceae to 52395fe Compare March 8, 2023 21:13
@smackesey smackesey marked this pull request as ready for review March 8, 2023 21:18
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from f4f2d4c to af156ec Compare March 8, 2023 21:41
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 52395fe to fbd9a52 Compare March 8, 2023 21:41
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from af156ec to 23e2962 Compare March 8, 2023 22:10
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from fbd9a52 to a947323 Compare March 8, 2023 22:10
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 23e2962 to 6b2e9ca Compare March 9, 2023 22:55
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from a947323 to 26dea1e Compare March 9, 2023 22:55
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from 6b2e9ca to dbe9487 Compare March 9, 2023 23:17
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 26dea1e to 77a7256 Compare March 9, 2023 23:18
@smackesey smackesey force-pushed the sean/instance-weakref-error-on-access branch from dbe9487 to b9bc2eb Compare March 10, 2023 00:43
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 77a7256 to f0430c0 Compare March 10, 2023 00:43
Base automatically changed from sean/instance-weakref-error-on-access to master March 10, 2023 01:18
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch 4 times, most recently from 78cabc6 to 328b2df Compare March 10, 2023 11:26
dpeng817
dpeng817 reviewed Mar 10, 2023
View reviewed changes
python_modules/dagster/dagster/_core/telemetry.py
Comment on lines +96 to +97
P = ParamSpec("P")
T = TypeVar("T")
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to have some documentation here, or aliases for the chains of callables. Even as someone who has dealt a lot with the telemetry code, finding it very difficult to figure out what I'm looking at.

Copy link
Collaborator Author

@smackesey smackesey Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah this is a bit dense-- the signatures of higher order decorators always are. I think it's considerably clearer when we use the style of our user-facing higher-order decorators, separating the target_fn and metadata args.

Unfortunately there's no way around the many uses of Callable[P, T] here without changing implementation (even then I'm not sure it would work).

dpeng817
dpeng817 requested changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

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

Would ideally like things to be a bit more readable around @telemetry_wrapper for example.

@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 328b2df to 2c02c3d Compare March 10, 2023 19:27
@smackesey smackesey force-pushed the sean/typing-static-telemetry branch from 2c02c3d to 05ecb23 Compare March 10, 2023 19:32
dpeng817
dpeng817 approved these changes Mar 12, 2023
View reviewed changes
Copy link
Contributor

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

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

I think the target_fn bit makes things clearer - appreciated

@smackesey smackesey merged commit 49180eb into master Mar 12, 2023
@smackesey smackesey deleted the sean/typing-static-telemetry branch March 12, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpeng817 dpeng817 dpeng817 approved these changes

@alangenfeld alangenfeld Awaiting requested review from alangenfeld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smackesey @dpeng817