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

Extract request id handling to dedicated class so it can be injected without DB dependency #31235

Merged
merged 5 commits into from
Mar 22, 2022

Conversation

nickvergessen
Copy link
Member

Logs before (with query_log_file):

SELECT * FROM `oc_appconfig`
SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED
SET SESSION AUTOCOMMIT=1
SELECT `class`, `entity`, CAST(`events` AS CHAR) AS `events` FROM `oc_flow_operations` WHERE `events` <> :dcValue1 GROUP BY `class`, `entity`, CAST(`events` AS CHAR)

Logs after (with query_log_file and query_log_file_requestid === yes)

jjI3eJAK1Ljz1OptLtpG	SELECT * FROM `oc_appconfig`
jjI3eJAK1Ljz1OptLtpG	SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED
jjI3eJAK1Ljz1OptLtpG	SET SESSION AUTOCOMMIT=1
jjI3eJAK1Ljz1OptLtpG	SELECT `class`, `entity`, CAST(`events` AS CHAR) AS `events` FROM `oc_flow_operations` WHERE `events` <> :dcValue1 GROUP BY `class`, `entity`, CAST(`events` AS CHAR)

Required for more in depth debugging on parallel requests

@nickvergessen nickvergessen added this to the Nextcloud 24 milestone Feb 16, 2022
@nickvergessen nickvergessen requested review from PVince81, CarlSchwan, a team and juliusknorr and removed request for a team February 16, 2022 22:50
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests needs to be adapted, but other than a few small nitpicks this looks fine to me.

In some ways, this feels a bit like duplicated work with #30823 since the requests are also stored with the requested (and arguments + execution time) there :/

We might want to discuss how we want to move forward since there is also OCP\Diagnostics too that also logs queries and having 3 DB requests loggers is maybe a bit too much

lib/private/Server.php Outdated Show resolved Hide resolved
lib/private/Server.php Outdated Show resolved Hide resolved
lib/private/DB/Connection.php Outdated Show resolved Hide resolved
@nickvergessen
Copy link
Member Author

In some ways, this feels a bit like duplicated work with #30823 since the requests are also stored with the requested (and arguments + execution time) there :/

Happy to talk about it. The problem is the profiler only helps in an isolated testing case. There is no way to e.g. monitor the full company call with it.

We might want to discuss how we want to move forward since there is also OCP\Diagnostics too that also logs queries and having 3 DB requests loggers is maybe a bit too much

That's also right. I need to checkout the Diagnostics stuff again what it does and can do.

…manually

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the techdebt/noid/extract-request-id branch from a30a3a2 to d078d53 Compare February 23, 2022 10:02
@nickvergessen nickvergessen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 23, 2022
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen merged commit 0acd4b5 into master Mar 22, 2022
@nickvergessen nickvergessen deleted the techdebt/noid/extract-request-id branch March 22, 2022 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants