introduce subclasses and add more telemetry for ProgrammingErrors #1651

sfc-gh-mchok · 2024-10-01T20:48:41Z

Pre-review checklist

I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
I've added or updated automated unit tests to verify correctness of my new code.
I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
I've confirmed that my changes are working by executing CLI's commands manually on Windows.
I've confirmed that my changes are up-to-date with the target branch.
I've described my changes in the release notes.
I've added or updated integration tests to verify correctness of my new code.
I've described my changes in the section below.

Changes description

Implemented custom errors derived from ClickException to replace places where we raise ProgrammingError
Added more telemetry (errno, sqlstate, error cause) to error events
Add shared code ownership to a couple of shared test files

src/snowflake/cli/_app/telemetry.py

tests/test_data/projects/napp_project_with_incorrect_pkg_warehouse/snowflake.yml

tests_integration/nativeapp/test_telemetry_errors.py

src/snowflake/cli/_app/telemetry.py

src/snowflake/cli/api/entities/utils.py

tests/app/test_telemetry.py

tests/test_data/projects/napp_project_with_incorrect_pkg_warehouse/snowflake.yml

src/snowflake/cli/_app/telemetry.py

src/snowflake/cli/_plugins/nativeapp/exceptions.py

src/snowflake/cli/api/entities/utils.py

src/snowflake/cli/api/exceptions.py

tests_integration/conftest.py

sfc-gh-turbaszek · 2024-10-15T13:27:14Z

src/snowflake/cli/api/exceptions.py

+class ShowSpecificObjectMultipleRowsError(ClickException):
+    def __init__(self, show_obj_query: str):
+        super().__init__(
+            f"Received multiple rows from result of SQL statement: {show_obj_query}. Usage of 'show_specific_object' may not be properly scoped."
+        )


Is this user facing error? How should they act if they see "Usage of 'show_specific_object' may not be properly scoped." message?

Just looking through the code, it seems like this is an error that is meant for us who is using the function, not the end users, and as the message says we need to make sure that our usage of this function is properly scoped (e.g. passing the right parameters)

sfc-gh-turbaszek · 2024-10-15T13:31:12Z

Should we use generic_sql_error_handler for SqlExecutionMixin_execute_string or in SnowTyper.post_execute where we rewrite db errors into click exceptions?

sfc-gh-mchok · 2024-10-15T15:49:36Z

Should we use generic_sql_error_handler for SqlExecutionMixin_execute_string or in SnowTyper.post_execute where we rewrite db errors into click exceptions?

@sfc-gh-turbaszek We are currently in the process of refactoring our error handling and one of our goals is actually to phase out the usage of generic_sql_error_handler since we want our error handling to be more specific and take into the context of when the error happened in order to provide more rich error messages/handling

…li exceptions, add catch for post deploy scripts

Updates tests in `test_manager.py` to use a v2 test project and call out to the v2 entities directly instead of going through the `NativeAppManager`. I've kept the diff as simple as possible and avoided introducing too many helpers or reorganizing the code, that can be done separately if necessary (the top priority is to just work towards removing `NativeAppManager` and all these static methods first).

Convert package post-deploy tests and one skipped integration tests to remove use of `NativeAppManager`.

…rror

This reverts commit 1b7072f. Since the PR that once depended on this one is now branching off main, we are re-adding the telemetry additions.

src/snowflake/cli/_app/telemetry.py

…rror

sfc-gh-mchok added 16 commits October 1, 2024 16:48

introduce subclasses for ProgrammingErrors and add more telemetry

98593ca

Merge branch 'main' into mchok-improve-ProgrammingError

aa91db5

fix unit tests and chain programmingerror for new errors

9d3109f

add test for attaching errno and sqlstate from programming error

9a07ed1

Merge branch 'main' into mchok-improve-ProgrammingError

38a4a27

use telemetry field constants in test to make it more robust

f557148

add test for cli exception classification

f505bf4

remove unnecessary fixtures from test

01f9d54

add cause in telemetry and rework tests

546c378

separate asserts for better tracing

7e94e5b

Merge branch 'main' into mchok-improve-ProgrammingError

3eda922

extract common message telemetry test util function

756e481

top level import causing test fails to local import

9e1755e

Merge branch 'main' into mchok-improve-ProgrammingError

b5d3209

add test for not programming error

4d58f96

Merge branch 'main' into mchok-improve-ProgrammingError

8e327f8

sfc-gh-mchok marked this pull request as ready for review October 3, 2024 17:54

sfc-gh-mchok requested review from a team as code owners October 3, 2024 17:54

sfc-gh-mchok changed the title ~~introduce subclasses for ProgrammingErrors and add more telemetry~~ introduce subclasses and add more telemetry for ProgrammingErrors Oct 3, 2024

sfc-gh-fcampbell reviewed Oct 3, 2024

View reviewed changes

sfc-gh-pjafari reviewed Oct 3, 2024

View reviewed changes

sfc-gh-bdufour reviewed Oct 3, 2024

View reviewed changes

sfc-gh-mchok added 3 commits October 4, 2024 14:51

rewrite tests using factories, fix telemetry logic

5178809

Merge branch 'main' into mchok-improve-ProgrammingError

89cd6bf

make errors more generic

9f31bcb

sfc-gh-mchok requested review from sfc-gh-bdufour, sfc-gh-fcampbell and sfc-gh-pjafari October 4, 2024 20:58

sfc-gh-bdufour reviewed Oct 7, 2024

View reviewed changes

tests_integration/conftest.py Outdated Show resolved Hide resolved

tests_integration/conftest.py Outdated Show resolved Hide resolved

sfc-gh-turbaszek reviewed Oct 15, 2024

View reviewed changes

sfc-gh-mchok added 2 commits October 15, 2024 10:07

Merge branch 'main' into mchok-improve-ProgrammingError

35f149f

remove telemetry additions to merge partial changes

1b7072f

sfc-gh-mchok dismissed sfc-gh-fcampbell’s stale review via 1b7072f October 15, 2024 15:20

sfc-gh-mchok changed the title ~~introduce subclasses and add more telemetry for ProgrammingErrors~~ refactor ProgrammingErrors being raised into subclasses Oct 15, 2024

sfc-gh-mchok added 2 commits October 15, 2024 11:31

Merge branch 'main' into mchok-improve-ProgrammingError

ac9fea7

Merge branch 'main' into mchok-improve-ProgrammingError

cdb2fcd

sfc-gh-mchok and others added 10 commits October 15, 2024 15:24

convert some errors to non-clickexception, add keyboardinterrupt to c…

de60c80

…li exceptions, add catch for post deploy scripts

Update more tests to get rid of NativeAppManager (#1724)

bba15a9

Convert package post-deploy tests and one skipped integration tests to remove use of `NativeAppManager`.

Merge remote-tracking branch 'origin' into mchok-improve-ProgrammingE…

682a18c

…rror

refactor tests to be in line with new changes

f800bb9

Merge remote-tracking branch 'origin' into mchok-improve-ProgrammingE…

1d088d0

…rror

Merge branch 'main' into mchok-improve-ProgrammingError

3772843

Revert "remove telemetry additions to merge partial changes"

ae7d566

This reverts commit 1b7072f. Since the PR that once depended on this one is now branching off main, we are re-adding the telemetry additions.

Merge branch 'main' into mchok-improve-ProgrammingError

886305e

Merge branch 'main' into mchok-improve-ProgrammingError

050932c

sfc-gh-mchok changed the title ~~refactor ProgrammingErrors being raised into subclasses~~ introduce subclasses and add more telemetry for ProgrammingErrors Oct 17, 2024

sfc-gh-mchok added 3 commits October 17, 2024 13:07

Merge branch 'main' into mchok-improve-ProgrammingError

c1f6778

remove specific post deploy script error for future refactor

e087539

get rid of unused context var

c436ccf

sfc-gh-melnacouzi reviewed Oct 17, 2024

View reviewed changes

src/snowflake/cli/_app/telemetry.py Outdated Show resolved Hide resolved

src/snowflake/cli/_app/telemetry.py Show resolved Hide resolved

src/snowflake/cli/_app/telemetry.py Show resolved Hide resolved

sfc-gh-mchok added 2 commits October 17, 2024 14:23

convert key present check to value not falsy check

e16f282

Merge remote-tracking branch 'origin' into mchok-improve-ProgrammingE…

7a6bdee

…rror

sfc-gh-melnacouzi approved these changes Oct 17, 2024

View reviewed changes

sfc-gh-turbaszek approved these changes Oct 18, 2024

View reviewed changes

sfc-gh-turbaszek merged commit ac6d4d1 into main Oct 18, 2024
18 checks passed

sfc-gh-turbaszek deleted the mchok-improve-ProgrammingError branch October 18, 2024 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce subclasses and add more telemetry for ProgrammingErrors #1651

introduce subclasses and add more telemetry for ProgrammingErrors #1651

sfc-gh-mchok commented Oct 1, 2024 •

edited

Loading

sfc-gh-turbaszek Oct 15, 2024

sfc-gh-mchok Oct 15, 2024

sfc-gh-turbaszek commented Oct 15, 2024

sfc-gh-mchok commented Oct 15, 2024 •

edited

Loading

introduce subclasses and add more telemetry for ProgrammingErrors #1651

introduce subclasses and add more telemetry for ProgrammingErrors #1651

Conversation

sfc-gh-mchok commented Oct 1, 2024 • edited Loading

Pre-review checklist

Changes description

sfc-gh-turbaszek Oct 15, 2024

Choose a reason for hiding this comment

sfc-gh-mchok Oct 15, 2024

Choose a reason for hiding this comment

sfc-gh-turbaszek commented Oct 15, 2024

sfc-gh-mchok commented Oct 15, 2024 • edited Loading

sfc-gh-mchok commented Oct 1, 2024 •

edited

Loading

sfc-gh-mchok commented Oct 15, 2024 •

edited

Loading