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

[CT-1904] Consolidate code execution result events, include adapter_response #6731

Closed
jtcohen6 opened this issue Jan 25, 2023 · 2 comments
Closed
Labels
enhancement New feature or request logging stale Issues that have gone stale

Comments

@jtcohen6
Copy link
Contributor

Consolidate event types

We added CodeExecution, CodeExecutionStatus, and the log_code_execution decorator when we added Python models:

def log_code_execution(code_execution_function):
# decorator to log code and execution time
if code_execution_function.__name__ != "submit_python_job":
raise ValueError("this should be only used to log submit_python_job now")
def execution_with_log(*args):
self = args[0]
connection_name = self.connections.get_thread_connection().name
fire_event(CodeExecution(conn_name=connection_name, code_content=args[2]))
start_time = time.time()
response = code_execution_function(*args)
fire_event(
CodeExecutionStatus(
status=response._message, elapsed=round((time.time() - start_time), 2)
)
)
return response
return execution_with_log

We should replace/consolidate SQLQuery + SQLQueryStatus to use those instead.

Include adapter_response for all query results

copying this part from #5325

Update SQLQueryStatus to take in the full adapter_response object, and node name. Today we stringify the adapter reponse first making it much harder to consume. Special attention will have to be paid to serialization since each warehouse will be putting different values into their own responses. Using examples from today's warehouses for inputs to serialization tests is a good idea. The name of the currently running node is accessible via the "jinja god context" (TODO: link example of how to access the jinja god context) (TODO: adapter.execute is called in other places too. We should add log lines there too. Link exact lines here.)

In my words:

  • I would like to see CodeExecutionStatus include the full AdapterResponse object (serialized to dict), rather than just the stringified message.
  • We also already include adapter_response in RunResult, but not every query execution actually yields a RunResult, only the last query for a given node.
@jtcohen6 jtcohen6 added enhancement New feature or request logging labels Jan 25, 2023
@github-actions github-actions bot changed the title Consolidate code execution result events, include adapter_response [CT-1904] Consolidate code execution result events, include adapter_response Jan 25, 2023
Copy link
Contributor

github-actions bot commented Feb 5, 2024

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days.

@github-actions github-actions bot added the stale Issues that have gone stale label Feb 5, 2024
Copy link
Contributor

Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest. Just add a comment to notify the maintainers.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request logging stale Issues that have gone stale
Projects
None yet
Development

No branches or pull requests

1 participant