-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
[datesets] feat: add statsd to datasets api #9577
[datesets] feat: add statsd to datasets api #9577
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a comment
superset/datasets/api.py
Outdated
@@ -344,6 +352,7 @@ def export(self, **kwargs: Any) -> Response: | |||
|
|||
@expose("/<pk>/refresh", methods=["PUT"]) | |||
@protect() | |||
@statsd_metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the position, after the safe
decorator
Codecov Report
@@ Coverage Diff @@
## master #9577 +/- ##
==========================================
+ Coverage 65.62% 70.47% +4.85%
==========================================
Files 574 574
Lines 30060 30081 +21
Branches 3054 3054
==========================================
+ Hits 19726 21199 +1473
+ Misses 10150 8771 -1379
+ Partials 184 111 -73
Continue to review full report at Codecov.
|
Can you please fill the "Summary" to add a bit of context to this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just missing 2 metric assertions
@@ -662,7 +662,7 @@ def test_dataset_item_refresh(self): | |||
|
|||
def test_dataset_item_refresh_not_found(self): | |||
""" | |||
Dataset API: Test item refresh not found dataset | |||
Dataset API: Test item refresh not found dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the metric assert here?
@@ -673,7 +673,7 @@ def test_dataset_item_refresh_not_found(self): | |||
|
|||
def test_dataset_item_refresh_not_owned(self): | |||
""" | |||
Dataset API: Test item refresh not owned dataset | |||
Dataset API: Test item refresh not owned dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the metric assert here?
454b4f8
to
095e76c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test plan to the PR as well? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, makes sense to me. thanks for the test plan!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
CATEGORY
Choose one
SUMMARY
Adding statds metrics to datesets API endpoints (POST, PUT, DELETE, bulk_delete, data). This is an extension from #9519
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
superset.DatasetRestApi.[endpoints]
overenvironment:sandbox
ADDITIONAL INFORMATION
REVIEWERS