-
Notifications
You must be signed in to change notification settings - Fork 58
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
Changes from 4 commits
a2a833e
46a3028
cf3a339
d26af10
6b3d12e
ca990d4
4edd8b2
f99a275
db00d94
84209f0
506bae2
7846c43
ec44e0c
d5e2376
15955f7
1b1d99a
a32a21b
b77d135
cef9f19
8a45192
bf8cf5b
ae43f25
93e5dc9
bfc4f21
5e1e38c
348aac2
c618da6
2fbdcb6
05d81a9
e96a0ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,7 @@ def phase( | |
self, | ||
enter_message: str, | ||
exit_message: Optional[str] = None, | ||
span_name: Optional[str] = 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. not sure if this is truly needed, since we can use multiple context managers on the same line. Thoughts? 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. I forgot about that feature, I adjusted these instances, thanks for the reminder |
||
) -> Iterator[Callable[[str], None]]: | ||
"""A context manager for organising steps into logical group.""" | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,11 +14,12 @@ | |
|
||
from __future__ import annotations | ||
|
||
from contextlib import contextmanager | ||
from contextlib import contextmanager, nullcontext | ||
from typing import Optional | ||
|
||
from rich.style import Style | ||
from rich.text import Text | ||
from snowflake.cli.api.cli_global_context import get_cli_context | ||
from snowflake.cli.api.console.abc import AbstractConsole | ||
from snowflake.cli.api.console.enum import Output | ||
|
||
|
@@ -68,7 +69,12 @@ 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( | ||
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. you're no longer touching this, I'd revert the file entirely |
||
self, | ||
enter_message: str, | ||
exit_message: Optional[str] = None, | ||
span_name: 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.") | ||
|
@@ -81,7 +87,10 @@ def phase(self, enter_message: str, exit_message: Optional[str] = None): | |
self._in_phase = True | ||
|
||
try: | ||
yield self.step | ||
with get_cli_context().metrics.start_span( | ||
span_name | ||
) if span_name else nullcontext(): | ||
yield self.step | ||
finally: | ||
self._in_phase = False | ||
if exit_message: | ||
|
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.
there's no compilation happening. How about simply "artifact_processors"? We should probably also have individual processors as sub-spans.