Adding timing function and fixes for logging the viz timings#14827
Adding timing function and fixes for logging the viz timings#14827mergify[bot] merged 17 commits intodevelopfrom
Conversation
JaroslavTulach
left a comment
There was a problem hiding this comment.
Pull Request Description
Added Duration.log_execution_time.
Why do we need Duration.log_execution_time? I am asking as I don't understand motivation for this change.
Probably an important question to start with: Who will consume these logged messages?
I'd prefer if visualization logging would require less changes in Standard.Base. I may not get the motivation right, but seems to me that even the log_execution_time shouldn't be in Duration - it is not an essential method of Duration - shouldn't it be in some Profiling API?
To finish my heresy: Why should every visualization be forced to think about log_execution_time? Shouldn't it be a responsibility of those who invoke the visualization to do some performance measurement and some logging automatically?
Again, this last question probably demonstrates I don't have a slight idea why this change is needed...
Added `Profile` to `Logging`.
Use ENSO_PROFILE_SQL to get timing now.
63f0289 to
109f06f
Compare
Use ENSO_PROFILE_SQL to get timing now. Docs
Fixes. Bug fix for Nanoseconds in Table viz.
| private: true | ||
| --- | ||
| Truncated display text for logging | ||
| private short_display value = if value.is_error then "<ERROR>" else |
There was a problem hiding this comment.
This is yet another variant in addition to to_text, to_display_text, pretty. Can't we just redefine to_display_text (for example) instead?
| Should be control by an environment variable set to the log level: | ||
| - `ENSO_PROFILE_VIZZES` - for visualizations. | ||
| - `ENSO_PROFILE_SQL` - for SQL queries. | ||
| - `ENSO_PROFILE_TABLE` - for table function calls. |
There was a problem hiding this comment.
It is weird that a generic utility for profiling references SQL, Table, vizzes... but so far I know no better way...
| # E.g. [Start] [End] [SQLite] SELECT * FROM "test" | ||
| padded_query = make_label.pad 100 | ||
| log_line = "[" + start + "] [" + Date_Time.now.to_text + "] " + padded_query + '\n' | ||
| Context.Output.with_enabled <| |
There was a problem hiding this comment.
- I don't like this manipulation with files in the profiled code at all!
- what's the use case? Is it:
when Enso is launched with some special variable, then a dedicated file with profiling info for particular area of code is created?
- if so, then I'd suggest to enhance the logging framework in the
ensolauncher itself to recognize such setting and act accordingly - something like:
$ ENSO_LOG_PROFILING=Standard.Table.Sql ./enso ...
$ cat ~/.local/share/enso/logs/profiling-Standard.Table.Sql.log
There was a problem hiding this comment.
You're correct, shouldn't have been in the profile time - have amended that.
The old SQL log function is used in a test and has been useful for diagnostics of DB work.
Removed from the profile, but leaving the old functionality as I don't want to rewrite the test at the moment.
|
|
||
| duration = pair.first | ||
| suffix = if suffix_callback == Nothing then "" else suffix_callback pair.second | ||
| module.log_message ("Execution time for " + label + ": " + duration.to_text + suffix) level |
There was a problem hiding this comment.
- I am puzzled: Why do we log here when the client code is also writing down files with similar messages by itself?
- either the
Profilingis inherently associated withLoggingand then the client code shall not do any further processing of logged messages - or
Profilingjust measures time and then it should be disassociated formLoggingcompletely and just measure time

Pull Request Description
Enhancing the existing visualization and widget profiling functionality. Allowing for a more full diagnostic of execution of a complex workflow.
Profile.time_executionas a single API for profiling functions ontoDuration.time_execution.ENSO_PROFILE_VIZZES,ENSO_PROFILE_SQL.ENSO_SQL_LOG_PATHto still log the SQL but with a new timestamp.Tablefunctions controlled byENSO_PROFILE_TABLE.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.