-
Notifications
You must be signed in to change notification settings - Fork 54
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
implement metrics counter feature tracking for NADE #1597
Open
sfc-gh-mchok
wants to merge
21
commits into
main
Choose a base branch
from
mchok-metrics-counter-feature-tracking
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+724
−4
Open
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
7f2618a
wip: implement metrics and implementation, tests failing
sfc-gh-mchok bd93cc3
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok 4470cd6
fix missing counter field causing test fails
sfc-gh-mchok b6101ab
add pdf template instrumentation
sfc-gh-mchok 945bc3c
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok 5ae5bc7
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok fe6926d
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok 61dd177
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok 9145b67
remove validate option callback for failing tests
sfc-gh-mchok 9dccb93
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok 949edf0
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok e8a3956
remove duplicate tracked counters, fix bug with carryover metrics bet…
sfc-gh-mchok 1316163
add integration tests for feature tracking metrics
sfc-gh-mchok 95ae58c
fix improper typing for None
sfc-gh-mchok d24aedb
implement metrics custom equality comparison for failing test
sfc-gh-mchok 674decd
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok c41044e
review fixes
sfc-gh-mchok f2def23
extract metrics update from render_definition_template
sfc-gh-mchok ef26a09
fix key being used for metrics
sfc-gh-mchok a17a0d4
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok 6cc8c5f
Merge branch 'main' into mchok-metrics-counter-feature-tracking
sfc-gh-mchok File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
# Copyright (c) 2024 Snowflake Inc. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from typing import Dict, Optional | ||
|
||
_FEATURES_PREFIX = "features" | ||
_APP_PREFIX = "app" | ||
|
||
|
||
class CLICounterField: | ||
TEMPLATES_PROCESSOR = f"{_FEATURES_PREFIX}.templates_processor" | ||
SQL_TEMPLATES = f"{_FEATURES_PREFIX}.sql_templates" | ||
PDF_TEMPLATES = f"{_FEATURES_PREFIX}.pdf_templates" | ||
SNOWPARK_PROCESSOR = f"{_FEATURES_PREFIX}.{_APP_PREFIX}.snowpark_processor" | ||
POST_DEPLOY_SCRIPTS = f"{_FEATURES_PREFIX}.{_APP_PREFIX}.post_deploy_scripts" | ||
|
||
|
||
class CLIMetrics: | ||
""" | ||
Class to track various metrics across the execution of a command | ||
""" | ||
|
||
def __init__(self): | ||
self._counters: Dict[str, int] = {} | ||
|
||
def __eq__(self, other): | ||
if isinstance(other, CLIMetrics): | ||
return self._counters == other._counters | ||
return False | ||
Comment on lines
+37
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need this? |
||
|
||
def get_counter(self, name: str) -> Optional[int]: | ||
return self._counters.get(name) | ||
|
||
def set_counter(self, name: str, value: int) -> None: | ||
self._counters[name] = value | ||
|
||
def add_counter(self, name: str, value: int) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
if name not in self._counters: | ||
self.set_counter(name, value) | ||
else: | ||
self._counters[name] += value | ||
|
||
@property | ||
def counters(self) -> Dict[str, int]: | ||
# return a copy of the original dict to avoid mutating the original | ||
return self._counters.copy() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
# Copyright (c) 2024 Snowflake Inc. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from snowflake.cli.api.metrics import CLIMetrics | ||
|
||
|
||
def test_metrics_no_counters(): | ||
# given | ||
metrics = CLIMetrics() | ||
|
||
# when | ||
|
||
# then | ||
assert metrics.counters == {} | ||
assert metrics.get_counter("counter1") is None | ||
|
||
|
||
def test_metrics_set_one_counter(): | ||
# given | ||
metrics = CLIMetrics() | ||
|
||
# when | ||
metrics.set_counter("counter1", 1) | ||
|
||
# then | ||
assert metrics.counters == {"counter1": 1} | ||
assert metrics.get_counter("counter1") == 1 | ||
|
||
|
||
def test_metrics_add_new_counter(): | ||
# given | ||
metrics = CLIMetrics() | ||
|
||
# when | ||
metrics.add_counter("counter1", 2) | ||
|
||
# then | ||
assert metrics.counters == {"counter1": 2} | ||
assert metrics.get_counter("counter1") == 2 | ||
|
||
|
||
def test_metrics_add_existing_counter(): | ||
# given | ||
metrics = CLIMetrics() | ||
|
||
# when | ||
metrics.set_counter("counter1", 2) | ||
metrics.add_counter(name="counter1", value=1) | ||
|
||
# then | ||
assert metrics.counters == {"counter1": 3} | ||
assert metrics.get_counter("counter1") == 3 | ||
|
||
|
||
def test_metrics_set_multiple_counters(): | ||
# given | ||
metrics = CLIMetrics() | ||
|
||
# when | ||
metrics.set_counter("counter1", 1) | ||
metrics.set_counter("counter2", 0) | ||
metrics.set_counter(name="counter2", value=2) | ||
|
||
# then | ||
assert metrics.counters == {"counter1": 1, "counter2": 2} | ||
assert metrics.get_counter("counter1") == 1 | ||
assert metrics.get_counter("counter2") == 2 |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I am wondering if we should switch these, so app comes first, so we can separate these easily? @sfc-gh-bdufour , @sfc-gh-turbaszek wdyt?
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.
What's here makes sense to me, I don't think I understand the suggestion. Can you explain?