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.

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

instrument NADE commands using metric spans #1844

Merged
merged 30 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a2a833e
draft PR for feedback on instrumentation
sfc-gh-mchok Nov 8, 2024
46a3028
remove changes on loggers
sfc-gh-mchok Nov 8, 2024
cf3a339
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 8, 2024
d26af10
extract enum value for proper key to message dict
sfc-gh-mchok Nov 8, 2024
6b3d12e
write custom decorator for spans, add tests
sfc-gh-mchok Nov 11, 2024
ca990d4
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 11, 2024
4edd8b2
rename spans as logical steps, remove extra param from console phase,…
sfc-gh-mchok Nov 12, 2024
f99a275
adjust tests for new spans
sfc-gh-mchok Nov 12, 2024
db00d94
time.monotonic() -> time.perf_counter() for proper ordering on windows
sfc-gh-mchok Nov 12, 2024
84209f0
add span for getting snowsight url since it is a blind spot
sfc-gh-mchok Nov 12, 2024
506bae2
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 13, 2024
7846c43
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 13, 2024
ec44e0c
Merge remote-tracking branch 'origin' into mchok-metric-spans-instrum…
sfc-gh-mchok Nov 13, 2024
d5e2376
add event sharing counters from merge conflicts to new file
sfc-gh-mchok Nov 13, 2024
15955f7
add span for create_or_upgrade_app to address blind spot
sfc-gh-mchok Nov 13, 2024
1b1d99a
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 14, 2024
a32a21b
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 14, 2024
b77d135
adjust span names for consistency
sfc-gh-mchok Nov 14, 2024
cef9f19
add docstring for entity action span decorator
sfc-gh-mchok Nov 14, 2024
8a45192
fix spans bundle test
sfc-gh-mchok Nov 14, 2024
bf8cf5b
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 18, 2024
ae43f25
sort spans deterministically by creation order
sfc-gh-mchok Nov 18, 2024
93e5dc9
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 18, 2024
bfc4f21
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 18, 2024
5e1e38c
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 18, 2024
348aac2
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 19, 2024
c618da6
only one sort, remove counter
sfc-gh-mchok Nov 19, 2024
2fbdcb6
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 20, 2024
05d81a9
Merge branch 'main' into mchok-metric-spans-instrument-nade-cmds
sfc-gh-mchok Nov 20, 2024
e96a0ee
update comment for span decorator to reflect new method name
sfc-gh-mchok Nov 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/snowflake/cli/api/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,8 @@ class for holding metrics span data and encapsulating related operations
children: Set[CLIMetricsSpan] = field(init=False, default_factory=set)

# private state
# start time of the step from the monotonic clock in order to calculate execution time
_monotonic_start: float = field(
init=False, default_factory=lambda: time.monotonic()
)
# start time of the step from a performance counter in order to calculate execution time
_start_time: float = field(init=False, default_factory=time.perf_counter)
sfc-gh-bdufour marked this conversation as resolved.
Show resolved Hide resolved

def __hash__(self) -> int:
return hash(self.span_id)
Expand Down Expand Up @@ -147,7 +145,7 @@ def finish(self, error: Optional[BaseException] = None) -> None:
if error:
self.error = error

self.execution_time = time.monotonic() - self._monotonic_start
self.execution_time = time.perf_counter() - self._start_time

def to_dict(self) -> Dict:
"""
Expand Down Expand Up @@ -184,9 +182,10 @@ class CLIMetrics:
_in_progress_spans: List[CLIMetricsSpan] = field(init=False, default_factory=list)
# list of finished steps for telemetry to process
_completed_spans: List[CLIMetricsSpan] = field(init=False, default_factory=list)
# monotonic clock time of when this class was initialized to approximate when the command first started executing
_monotonic_start: float = field(
init=False, default_factory=lambda: time.monotonic(), compare=False
# clock time of a performance counter when this class was initialized
# to approximate when the command first started executing
_start_time: float = field(
init=False, default_factory=time.perf_counter, compare=False
)

def clone(self) -> CLIMetrics:
Expand Down Expand Up @@ -229,7 +228,7 @@ def span(self, name: str) -> Iterator[CLIMetricsSpan]:
"""
new_span = CLIMetricsSpan(
name=name,
start_time=time.monotonic() - self._monotonic_start,
start_time=time.perf_counter() - self._start_time,
parent=self.current_span,
)

Expand Down
19 changes: 4 additions & 15 deletions tests/api/test_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

import uuid
from itertools import count
from unittest import mock

import pytest
from snowflake.cli.api.metrics import (
Expand Down Expand Up @@ -110,14 +109,6 @@ def test_metrics_set_default_existing_counter():
assert metrics.counters == {"c2": 2}


# we need to mock time.monotonic because on windows it does not
# capture enough precision for these tests to not be flaky
@pytest.fixture
def mock_time_monotonic():
with mock.patch("time.monotonic", side_effect=count()) as mocked_time_monotonic:
yield mocked_time_monotonic


# helper for testing span limits
def create_spans(metrics: CLIMetrics, width: int, depth: int):
counter = count()
Expand Down Expand Up @@ -146,7 +137,7 @@ def test_metrics_spans_initialization_empty():
assert metrics.num_spans_past_total_limit == 0


def test_metrics_spans_single_span_no_error_or_parent(mock_time_monotonic):
def test_metrics_spans_single_span_no_error_or_parent():
# given
metrics = CLIMetrics()

Expand Down Expand Up @@ -191,7 +182,7 @@ def test_metrics_spans_finish_early_is_idempotent():
assert span1_dict[CLIMetricsSpan.EXECUTION_TIME_KEY] == execution_time


def test_metrics_spans_parent_with_one_child(mock_time_monotonic):
def test_metrics_spans_parent_with_one_child():
# given
metrics = CLIMetrics()

Expand Down Expand Up @@ -244,7 +235,7 @@ def test_metrics_spans_parent_with_one_child(mock_time_monotonic):
)


def test_metrics_spans_parent_with_two_children_same_name(mock_time_monotonic):
def test_metrics_spans_parent_with_two_children_same_name():
# given
metrics = CLIMetrics()

Expand Down Expand Up @@ -358,9 +349,7 @@ def test_metrics_spans_empty_name_raises_error():
assert err.match("span name must not be empty")


def test_metrics_spans_passing_depth_limit_should_add_to_counter_and_not_emit(
mock_time_monotonic,
):
def test_metrics_spans_passing_depth_limit_should_add_to_counter_and_not_emit():
# given
metrics = CLIMetrics()

Expand Down
Loading