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 10 commits
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
2 changes: 1 addition & 1 deletion .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
src/snowflake/cli/api/metrics.py @snowflakedb/snowcli @snowflakedb/nade
src/snowflake/cli/api/project/definition_conversion.py @snowflakedb/snowcli @snowflakedb/nade
src/snowflake/cli/api/utils/ @snowflakedb/snowcli @snowflakedb/nade
tests/api/metrics/ @snowflakedb/snowcli @snowflakedb/nade
tests/api/test_metrics.py @snowflakedb/snowcli @snowflakedb/nade
tests_common/__init__.py @snowflakedb/snowcli @snowflakedb/nade
tests_common/conftest.py @snowflakedb/snowcli @snowflakedb/nade
tests_common/deflake.py @snowflakedb/snowcli @snowflakedb/nade
Expand Down
13 changes: 10 additions & 3 deletions src/snowflake/cli/_app/telemetry.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ class CLITelemetryField(Enum):
CONFIG_FEATURE_FLAGS = "config_feature_flags"
# Metrics
COUNTERS = "counters"
SPANS = "spans"
COMPLETED_SPANS = "completed_spans"
NUM_SPANS_PAST_DEPTH_LIMIT = "num_spans_past_depth_limit"
NUM_SPANS_PAST_TOTAL_LIMIT = "num_spans_past_total_limit"
# Information
EVENT = "event"
ERROR_MSG = "error_msg"
Expand Down Expand Up @@ -129,9 +133,12 @@ def _get_command_metrics() -> TelemetryDict:
cli_context = get_cli_context()

return {
CLITelemetryField.COUNTERS: {
**cli_context.metrics.counters,
}
CLITelemetryField.COUNTERS: cli_context.metrics.counters,
CLITelemetryField.SPANS: {
CLITelemetryField.COMPLETED_SPANS.value: cli_context.metrics.completed_spans,
CLITelemetryField.NUM_SPANS_PAST_DEPTH_LIMIT.value: cli_context.metrics.num_spans_past_depth_limit,
CLITelemetryField.NUM_SPANS_PAST_TOTAL_LIMIT.value: cli_context.metrics.num_spans_past_total_limit,
},
}


Expand Down
2 changes: 2 additions & 0 deletions src/snowflake/cli/_plugins/nativeapp/artifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from typing import Any, Callable, Dict, Iterable, Iterator, List, Optional, Tuple, Union

from click.exceptions import ClickException
from snowflake.cli.api.cli_global_context import span
from snowflake.cli.api.constants import DEFAULT_SIZE_LIMIT_MB
from snowflake.cli.api.project.schemas.v1.native_app.path_mapping import PathMapping
from snowflake.cli.api.project.util import to_identifier
Expand Down Expand Up @@ -648,6 +649,7 @@ def resolve_without_follow(path: Path) -> Path:
return Path(os.path.abspath(path))


@span("build_initial_bundle")
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just bundle ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my head bundle is the higher level container that does both this step (populating the deploy root) and then applying all the processors on it, but if we're fine just having the first step be called bundle then that's good with me

def build_bundle(
project_root: Path,
deploy_root: Path,
Expand Down
5 changes: 4 additions & 1 deletion src/snowflake/cli/_plugins/nativeapp/codegen/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ def compile_artifacts(self):
if not self._should_invoke_processors():
return

with cc.phase("Invoking artifact processors"):
with (
cc.phase("Invoking artifact processors"),
get_cli_context().metrics.span("artifact_processors"),
):
if self._bundle_ctx.generated_root.exists():
raise ClickException(
f"Path {self._bundle_ctx.generated_root} already exists. Please choose a different name for your generated directory in the project definition file."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
NativeAppExtensionFunction,
)
from snowflake.cli._plugins.stage.diff import to_stage_path
from snowflake.cli.api.cli_global_context import get_cli_context
from snowflake.cli.api.cli_global_context import get_cli_context, span
from snowflake.cli.api.console import cli_console as cc
from snowflake.cli.api.metrics import CLICounterField
from snowflake.cli.api.project.schemas.v1.native_app.path_mapping import (
Expand Down Expand Up @@ -167,6 +167,7 @@ class SnowparkAnnotationProcessor(ArtifactProcessor):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

@span("snowpark_processor")
def process(
self,
artifact_to_process: PathMapping,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
ArtifactProcessor,
)
from snowflake.cli._plugins.nativeapp.exceptions import InvalidTemplateInFileError
from snowflake.cli.api.cli_global_context import get_cli_context
from snowflake.cli.api.cli_global_context import get_cli_context, span
from snowflake.cli.api.console import cli_console as cc
from snowflake.cli.api.metrics import CLICounterField
from snowflake.cli.api.project.schemas.v1.native_app.path_mapping import (
Expand Down Expand Up @@ -91,6 +91,7 @@ def expand_templates_in_file(
if expanded_template != file.contents:
file.edited_contents = expanded_template

@span("templates_processor")
def process(
self,
artifact_to_process: PathMapping,
Expand Down
12 changes: 8 additions & 4 deletions src/snowflake/cli/_plugins/nativeapp/entities/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
from snowflake.cli._plugins.nativeapp.sf_facade import get_snowflake_facade
from snowflake.cli._plugins.nativeapp.utils import needs_confirmation
from snowflake.cli._plugins.workspace.context import ActionContext
from snowflake.cli.api.cli_global_context import span
from snowflake.cli.api.entities.common import EntityBase, get_sql_executor
from snowflake.cli.api.entities.utils import (
drop_generic_object,
Expand Down Expand Up @@ -146,6 +147,7 @@ def post_deploy_hooks(self) -> list[PostDeployHook] | None:
model = self._entity_model
return model.meta and model.meta.post_deploy

@span("deploy_app")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should consider some kind of standardized scheme for actions + entities, maybe something like action.app.deploy ?

def action_deploy(
self,
action_ctx: ActionContext,
Expand Down Expand Up @@ -230,6 +232,7 @@ def action_deploy(
interactive=interactive,
)

@span("drop_app")
def action_drop(
self,
action_ctx: ActionContext,
Expand Down Expand Up @@ -513,10 +516,10 @@ def create_or_upgrade_app(
create_cursor = sql_executor.execute_query(
dedent(
f"""\
create application {self.name}
from application package {package.name} {using_clause} {debug_mode_clause}
comment = {SPECIAL_COMMENT}
"""
create application {self.name}
from application package {package.name} {using_clause} {debug_mode_clause}
comment = {SPECIAL_COMMENT}
"""
),
)
print_messages(console, create_cursor)
Expand Down Expand Up @@ -769,6 +772,7 @@ def stream_events(
except KeyboardInterrupt:
return

@span("get_snowsight_url_for_app")
sfc-gh-fcampbell marked this conversation as resolved.
Show resolved Hide resolved
def get_snowsight_url(self) -> str:
"""Returns the URL that can be used to visit this app via Snowsight."""
name = identifier_for_url(self.name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
from snowflake.cli._plugins.stage.diff import DiffResult
from snowflake.cli._plugins.stage.manager import StageManager
from snowflake.cli._plugins.workspace.context import ActionContext
from snowflake.cli.api.cli_global_context import span
from snowflake.cli.api.entities.common import EntityBase, get_sql_executor
from snowflake.cli.api.entities.utils import (
drop_generic_object,
Expand Down Expand Up @@ -218,6 +219,7 @@ def action_deploy(
force=force,
)

@span("drop_app_package")
sfc-gh-fcampbell marked this conversation as resolved.
Show resolved Hide resolved
def action_drop(self, action_ctx: ActionContext, force_drop: bool, *args, **kwargs):
console = self._workspace_ctx.console
sql_executor = get_sql_executor()
Expand Down Expand Up @@ -548,6 +550,7 @@ def _bundle(self):
compiler.compile_artifacts()
return bundle_map

@span("deploy_app_package")
Copy link
Contributor

Choose a reason for hiding this comment

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

would rather do this on the action for consistency

def _deploy(
self,
bundle_map: BundleMap | None,
Expand Down Expand Up @@ -915,6 +918,7 @@ def validate_setup_script(
if validation_result["status"] == "FAIL":
raise SetupScriptFailedValidation()

@span("validate_setup_script")
def get_validation_result(
self, use_scratch_stage: bool, interactive: bool, force: bool
):
Expand Down
23 changes: 22 additions & 1 deletion src/snowflake/cli/api/cli_global_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from contextlib import contextmanager
from contextvars import ContextVar
from dataclasses import dataclass, field, replace
from functools import wraps
from pathlib import Path
from typing import TYPE_CHECKING, Iterator

Expand Down Expand Up @@ -157,7 +158,7 @@ def enable_tracebacks(self) -> bool:
return self._manager.enable_tracebacks

@property
def metrics(self):
def metrics(self) -> CLIMetrics:
return self._manager.metrics

@property
Expand Down Expand Up @@ -213,6 +214,26 @@ def get_cli_context() -> _CliGlobalContextAccess:
return _CliGlobalContextAccess(get_cli_context_manager())


def span(span_name: str):
"""
Decorator to start a command metrics span that encompasses a whole function

Must be used instead of directly calling @get_cli_context().metrics.start_span(span_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's span now, not start_span

as a decorator to ensure that the cli context is grabbed at run time instead of at
module load time, which would not reflect forking
"""

def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
with get_cli_context().metrics.span(span_name):
return func(*args, **kwargs)

return wrapper

return decorator


@contextmanager
def fork_cli_context(
connection_overrides: dict | None = None,
Expand Down
6 changes: 5 additions & 1 deletion src/snowflake/cli/api/console/console.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ def _format_message(self, message: str, output: Output) -> Text:
return text

@contextmanager
def phase(self, enter_message: str, exit_message: Optional[str] = None):
def phase(
Copy link
Contributor

Choose a reason for hiding this comment

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

you're no longer touching this, I'd revert the file entirely

self,
enter_message: str,
exit_message: Optional[str] = None,
):
"""A context manager for organising steps into logical group."""
if self.in_phase:
raise CliConsoleNestingProhibitedError("Only one phase allowed at a time.")
Expand Down
7 changes: 5 additions & 2 deletions src/snowflake/cli/api/entities/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
to_stage_path,
)
from snowflake.cli._plugins.stage.utils import print_diff_to_console
from snowflake.cli.api.cli_global_context import get_cli_context
from snowflake.cli.api.cli_global_context import get_cli_context, span
from snowflake.cli.api.console.abc import AbstractConsole
from snowflake.cli.api.entities.common import get_sql_executor
from snowflake.cli.api.errno import (
Expand Down Expand Up @@ -76,6 +76,7 @@ def _get_stage_paths_to_sync(
return stage_paths


@span("sync_remote_and_local_files")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is one of the rare cases where I'd use the actual function name, since it's exactly the logical concept we're timing.

def sync_deploy_root_with_stage(
console: AbstractConsole,
deploy_root: Path,
Expand Down Expand Up @@ -219,7 +220,9 @@ def execute_post_deploy_hooks(

get_cli_context().metrics.set_counter(CLICounterField.POST_DEPLOY_SCRIPTS, 1)

with console.phase(f"Executing {deployed_object_type} post-deploy actions"):
with console.phase(
f"Executing {deployed_object_type} post-deploy actions",
), get_cli_context().metrics.span("post_deploy_hooks"):
sql_scripts_paths = []
display_paths = []
for hook in post_deploy_hooks:
Expand Down
19 changes: 9 additions & 10 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 @@ -216,7 +215,7 @@ def current_span(self) -> Optional[CLIMetricsSpan]:
return self._in_progress_spans[-1] if len(self._in_progress_spans) > 0 else None

@contextmanager
def start_span(self, name: str) -> Iterator[CLIMetricsSpan]:
def span(self, name: str) -> Iterator[CLIMetricsSpan]:
"""
Starts a new span that tracks various metrics throughout its execution

Expand All @@ -229,7 +228,7 @@ def start_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
Empty file removed tests/api/metrics/__init__.py
Empty file.
Loading
Loading