[SPEC] Add urAdapterSetLoggingCallback entrypoint #1
Conversation
| - type: $x_log_level_t | ||
| name: logLevel | ||
| desc: "[in] The log message type" |
There was a problem hiding this comment.
I feel like the log level should be set once as an argument to urAdapterSetLoggingCallback. This would mirror how it gets set by environment variable i.e. once per process. The main benefit of setting the log level once is to allow the logging framework to avoid paying the overhead of processing log messages which are going to be ignored.
There was a problem hiding this comment.
That makes sense. I added the extra parameter in the latest commit 👍
scripts/core/adapter.yml
Outdated
| - "Sets a function callback that will be used by the adapters when logging messages." | ||
| - "The callback will be called for every message logged by the adapters specified in `phAdapters`." | ||
| - "Sending log messages is optional and adapters are not required to implement this feature for any entrypoint." | ||
| - "The application can disable log messages at runtime by setting the environment variable `UR_DISABLE_LOGS`." |
There was a problem hiding this comment.
We should also define what happens when this entry point is called more than once. Do we replace the callback or append it to a list of callbacks?
I think replacing the callback would be simpler to implement. If that turns out to be an issue we can extend it later.
There was a problem hiding this comment.
I updated the wording on the description for this case. Also allowed the callback to be nullptr in case the application wants to unset it.
I'm not sure what to do with the environment variable UR_DISABLE_LOGS. With the logger already having its own env_vars, I think that this variable is a bit redundant (if the application wants to ignore all the logs they can just unset the callback). But would like to hear someone else's opinion. It's not hard to implement the env var if we still think it is relevant.
Temporarily creating the PR on top of a branch on my fork which contains the following unmerged PR (they are dependencies for this change): oneapi-src#1359, oneapi-src#1347, oneapi-src#1345, oneapi-src#1342, oneapi-src#1032