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

dev/core#2032 Add potential to vary query log files per process #18471

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 15, 2020

Overview

Creates option to redirect sql logging output to a new file

Before

All logging of sql queries goes to the same file, subject to log rotation

After

If a truthy value other than the reserved string 'backtrace' is used it will impact the eventual file name of the log.
e.g

env CIVICRM_DEBUG_LOG_QUERY=test_2 drush mycommand

Which would result in the created log file having a name like

CiviCRM.sql_log_test_2.7a880382d2e1d80611365ce1.log

Technical Details

This might be a bit too hacky but I thought I'd put it up for consideration. Basically we have 3 existing
options for CIVICRM_DEBUG_LOG_QUERY

  1. FALSE = do nothing (5.30 + or same as TRUE for earlier versions)
  2. backtrace = log a full backtrace
  3. anything else - log the sql query

This takes a bit of leeway with the 'everything else' and includes the value a part of the
prefix for writing to the query log - allowing something like

Comments

https://lab.civicrm.org/dev/core/-/issues/2032

@totten what is your opinion on this approach? We could add another define & I feel like I could make a case either way

This might be  a bit too hacky but I thought I'd put it up for consideration. Basically we have 3 existing
options for CIVICRM_DEBUG_LOG_QUERY

1) FALSE = do nothing (5.30 + or same as TRUE for earlier versions
2) backtrace = log a full backtrace
3) anything else - log the sql query

This takes a bit of leeway with the 'everything else' and includes the value a part of the
prefix for writing to the query log - allowing something like

``
env CIVICRM_DEBUG_LOG_QUERY=test_2 drush mycommand
``

Which would result in the created log file having a name like
```
CiviCRM.sql_log_test_2.7a880382d2e1d80611365ce1.log
```
@civibot
Copy link

civibot bot commented Sep 15, 2020

(Standard links)

@civibot civibot bot added the master label Sep 15, 2020
@totten
Copy link
Member

totten commented Sep 16, 2020

@eileenmcnaughton That seems pretty clever minimalism. For the CLI case, it seems workable - and I can't see any real problems. (If CIVICRM_DEBUG_LOG_QUERY were untrusted input, then there could be problems... but it is trusted input...)

It should be explained more wherever there's documentation for CIVICRM_DEBUG_LOG_QUERY. (Maybe civicrm.settings.php.template?)

The only question I have is how much we care about the web-based case -- e.g. if you're sending requests via web-browser and want to separate the SQL logs. In that case, it might make more sense to name files based on (a) session-id or (b) CRM_Utils_Request::id(). However, both those may be rabbit-holes (b/c SQL starts running early during bootstrap -- possibly before there's a session or request-id).

So I'd put it this way:

  • If you expect to also need to separate logs for web requests, then maybe it's worth the harder discussion/design to tackle both in a similar way.
  • If you're not expecting to have the web-case, then I think this is fine, and the pithiness is appealing.

@eileenmcnaughton
Copy link
Contributor Author

@totten I have no current use case for via web but it seems conceivable - we could just add it like

'sql_log' . CIVICRM_DEBUG_LOG_QUERY . $webBasedIdentifier

at some later point if it arises.

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-dev-docs that referenced this pull request Sep 16, 2020
Note there is more information in

https://lab.civicrm.org/extensions/systemtools/-/blob/master/README.md

As a follow up I could provide a link to that if wanted
@eileenmcnaughton
Copy link
Contributor Author

@totten I added a docs PR here civicrm/civicrm-dev-docs#853 - the civicrm.settings.php.template doesn't have any existing info about this define but dev-docs does so it seemed a better place

homotechsual added a commit to civicrm/civicrm-dev-docs that referenced this pull request Sep 16, 2020
* Adds information about the change in civicrm/civicrm-core#18471

Note there is more information in

https://lab.civicrm.org/extensions/systemtools/-/blob/master/README.md

As a follow up I could provide a link to that if wanted

* Update docs/tools/debugging.md

Co-authored-by: Mikey O'Toole <[email protected]>

Co-authored-by: Mikey O'Toole <[email protected]>
@eileenmcnaughton
Copy link
Contributor Author

docs pr is merged

@eileenmcnaughton
Copy link
Contributor Author

@totten OK to merge now?

@totten
Copy link
Member

totten commented Sep 22, 2020

Yup, let's merge based on the previous testing.

@totten totten merged commit d252387 into civicrm:master Sep 22, 2020
@eileenmcnaughton eileenmcnaughton deleted the error branch September 22, 2020 04:24
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-packages that referenced this pull request Oct 2, 2020
@eileenmcnaughton
Copy link
Contributor Author

Also requires civicrm/civicrm-packages#309

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants