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

[dagster-prometheus] Update dagster-prometheus using Pythonic resources w/ lifecycle hooks #13894

Merged
merged 9 commits into from
May 30, 2023
Merged

[dagster-prometheus] Update dagster-prometheus using Pythonic resources w/ lifecycle hooks #13894

merged 9 commits into from
May 30, 2023

Conversation

benpankow
Copy link
Member

@benpankow benpankow commented Apr 25, 2023 •
edited

Sumamry

Reimplementation of #13869 which migrates the dagster-prometheus resource as a configurable-resource w/ setup lifecycle hook that sets up some state on the class.

@benpankow
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@benpankow benpankow requested a review from schrockn April 25, 2023 19:52
@benpankow benpankow mentioned this pull request Apr 25, 2023
Closed
@benpankow benpankow requested a review from schrockn April 25, 2023 21:49
@benpankow benpankow force-pushed the benpankow/update-dagster-prometheus-rfc branch from cdc0437 to 34c2a47 Compare April 25, 2023 23:04
@github-actions
Copy link

github-actions bot commented Apr 25, 2023 •
edited

Deploy preview for dagster ready!

Preview available at https://dagster-3ertrudl4-elementl.vercel.app

Direct link to changed pages:

@benpankow benpankow mentioned this pull request Apr 25, 2023
Merged
benpankow added a commit that referenced this pull request May 3, 2023
@benpankow
[docs] Update state-in-resources docs to include lifecycle hooks (#13905
caaa905 
)

## Summary

Updates the resources docs to utilize the hooks in #13938, explaining a
basic case using `PrivateAttr` & `setup_for_execution` alongside a more
complex yield case.

Corresponds to the approach taken in #13894
benpankow added a commit that referenced this pull request May 3, 2023
@benpankow
[docs] Update state-in-resources docs to include lifecycle hooks (#13905
9ad1fe5 
)

## Summary

Updates the resources docs to utilize the hooks in #13938, explaining a
basic case using `PrivateAttr` & `setup_for_execution` alongside a more
complex yield case.

Corresponds to the approach taken in #13894
@benpankow benpankow force-pushed the benpankow/update-dagster-prometheus-rfc branch from 34c2a47 to d4b97b5 Compare May 25, 2023 11:25
@benpankow benpankow changed the title [dagster-prometheus][rfc] Update dagster-prometheus using Pythonic resources w/ post-init [dagster-prometheus] Update dagster-prometheus using Pythonic resources w/ lifecycle hooks May 25, 2023
@benpankow
Copy link
Member Author

Updated to use new lifecycle hooks.

schrockn
schrockn requested changes May 26, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Have some minor suggestions

This is also a bit of a goofy resource in that it take configuration but then that configuration does not impact the collector registry as far as I can tell. So the configuration only effects the methods we wrap, but then the user is also expected to use the prometheus APIs directly.

For example in our test we configure the resource but I don't think we use it all?

Just requesting changes to make sure you see this question.

python_modules/libraries/dagster-prometheus/dagster_prometheus/resources.py Outdated Show resolved Hide resolved
python_modules/libraries/dagster-prometheus/dagster_prometheus_tests/test_resources.py Outdated Show resolved Hide resolved
python_modules/libraries/dagster-prometheus/dagster_prometheus_tests/test_resources.py Outdated
Comment on lines 33 to 49
def test_prometheus_counter_pythonic_res() -> None:
@op
def prometheus_solid(prometheus: PrometheusResource) -> None:
c = Counter(
"some_counter_seconds",
"Description of this counter",
registry=prometheus.registry,
)
c.inc()
c.inc(1.6)
recorded = prometheus.registry.get_sample_value("some_counter_seconds_total")
assert recorded
assert abs(2.6 - recorded) < EPS

assert wrap_op_in_graph_and_execute(
prometheus_solid, run_config=ENV, resources=RESOURCES
).success
Copy link
Member

Choose a reason for hiding this comment

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

does this test even use the configuration at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's right. It is a bit silly as currently constructed. The tests don't touch on the codepaths that use the config (before and in this PR)

schrockn
schrockn requested changes May 26, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Have some minor suggestions

This is also a bit of a goofy resource in that it take configuration but then that configuration does not impact the collector registry as far as I can tell. So the configuration only effects the methods we wrap, but then the user is also expected to use the prometheus APIs directly.

For example in our test we configure the resource but I don't think we use it all?

Just requesting changes to make sure you see this question.

@benpankow benpankow force-pushed the benpankow/update-dagster-prometheus-rfc branch from 0203eb7 to 063aa4b Compare May 30, 2023 16:39
@github-actions
Copy link

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-14iy63sif-elementl.vercel.app

Built with commit 063aa4b.
This pull request is being automatically deployed with vercel-action

@github-actions
Copy link

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-a8ag1t6cx-elementl.vercel.app

Built with commit 063aa4b.
This pull request is being automatically deployed with vercel-action

@benpankow benpankow merged commit 439ca6c into master May 30, 2023
3 of 4 checks passed
@benpankow benpankow deleted the benpankow/update-dagster-prometheus-rfc branch May 30, 2023 17:16
odette-elementl pushed a commit that referenced this pull request Jun 1, 2023
@benpankow @odette-elementl
[dagster-prometheus] Update dagster-prometheus using Pythonic resourc…
a2f266e 
…es w/ lifecycle hooks (#13894)

## Sumamry

Reimplementation of #13869 which migrates the `dagster-prometheus`
resource as a configurable-resource w/ setup lifecycle hook that sets up
some state on the class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schrockn schrockn schrockn approved these changes

@erinkcochran87 erinkcochran87 Awaiting requested review from erinkcochran87

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@benpankow @schrockn