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

Blacklist: remember the X-Request-ID #6427

Merged
merged 2 commits into from
May 15, 2018
Merged

Blacklist: remember the X-Request-ID #6427

merged 2 commits into from
May 15, 2018

Conversation

ogoffart
Copy link
Contributor

@ogoffart ogoffart commented Apr 4, 2018

Issue #6420
Store the X-Request-ID in the SyncFileItem and also in the blacklist.
Note that for consistency reason, the X-Request-ID is also in the
SyncFileItem if the request succeeds.

Currently there is no UI to access it, but it can be queried with sql
commands

@ogoffart ogoffart requested a review from ckamm April 4, 2018 14:31
@ogoffart
Copy link
Contributor Author

ogoffart commented Apr 4, 2018

I'm a bit concerned about memory usage. Should we only keep the request id in case of faillure?

@SamuAlfageme
Copy link
Contributor

@ogoffart yes, having a QByteArray allocated for each SyncFileItem might be a bit too much (specially after reading the considerations on #5853 (comment) - client currently uses ~42bytes) - I'd say keeping it on memory up until a request resolves and only store it on disk on failure will be a better idea.

And I'd really like to have an option on the UI for inexperienced users (not savvy with sqlite commands) to fetch this info and send it to admins.

@guruz guruz added this to the 2.5.0 milestone Apr 5, 2018
@guruz
Copy link
Contributor

guruz commented Apr 5, 2018

I'm a bit concerned about memory usage.

Could avoid this by having the request ID be a crazy hash of the request, path, sync run iteration and PID and some other source. But maybe better not :-)

@ckamm
Copy link
Contributor

ckamm commented Apr 6, 2018

Personally I've never used X-Request-ID - putting it in the log I understand, as it doesn't cause other overhead. I don't currently see the ids as particularly useful - am I missing something where they are a huge win?

Storing in the blacklist is fine. But I am worried that we're spending a lot of work on a weak debugging tool.

Copy link
Contributor

@ckamm ckamm left a comment

Choose a reason for hiding this comment

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

Code looks ok

sqlFail("updateBlacklistTableStructure: Add requestId", query);
re = false;
}
commitInternal("update database structure: add errorCategory col");
Copy link
Contributor

Choose a reason for hiding this comment

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

requestId

@SamuAlfageme
Copy link
Contributor

@ckamm about:

putting it in the log I understand, as it doesn't cause other overhead. I don't currently see the ids as particularly useful

Main concerns I've heard are related to non-persistent-logs setups (user ends up with issues on the "Not Synced" tab but was not using logging). Then he can use those to help the admin/support to correlate and determine which request on the server logs (usually persistent) was responsible for the issue.

cc/ @michaelstingl

@ckamm
Copy link
Contributor

ckamm commented Apr 9, 2018

@SamuAlfageme Okay then. It's mostly that I've not personally been in a situation where correlating with server logs was useful and I wanted to understand where the need was coming from.

@ogoffart
Copy link
Contributor Author

@SamuAlfageme so where exactly do you want these ID to be stored? In which log?

@guruz
Copy link
Contributor

guruz commented Apr 23, 2018

@ogoffart new column in .owncloudsync.log?

@SamuAlfageme
Copy link
Contributor

@ogoffart on #6420 (comment) I thought on using sync journal's blacklist table. As @guruz points out, .owncloudsync.log would also make sense (and will even keep historical data from previous sync runs, even when the error was solved afterwards):

# timestamp | duration | file | instruction | dir | modtime | etag | size | fileId | status | errorString | http result code | other size | other modtime | other etag | other fileId | other instruction | X-Request-ID |
[...]
||Screenshot 2018-04-19 11.52.43.png|INST_NEW|Down|1524131563|2ba3a0df3bd29a417512fe01fce5e522|17901|00000029ocdkxjqy1s03|2|Server replied "403 Forbidden" to "GET https://localhost/remote.php/dav/files/admin/Screenshot 2018-04-19 11.52.43.png"|403|0|0||||4c5adb86-2a66-4450-9af9-f6f77b5bdf581522758593|
[...]

@guruz
Copy link
Contributor

guruz commented Apr 24, 2018

@ogoffart merge this and potentially do a follow up PR?

Issue #6420
Store the X-Request-ID in the SyncFileItem and also in the blacklist.
Note that for consistency reason, the X-Request-ID is also in the
SyncFileItem if the request succeeds.

Currently there is no UI to access it, but it can be queried with sql
commands
@guruz
Copy link
Contributor

guruz commented May 16, 2018

ˆˆ @SamuAlfageme FYI this is in now

@TheOneRing TheOneRing deleted the blacklist branch December 2, 2019 16:44
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Nov 27, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Nov 27, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Dec 2, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Dec 8, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Dec 10, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Dec 10, 2020
github-actions bot pushed a commit to nextcloud/desktop that referenced this pull request Dec 11, 2020
github-actions bot pushed a commit to nextcloud/desktop that referenced this pull request Dec 14, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Dec 14, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Dec 14, 2020
er-vin pushed a commit to nextcloud/desktop that referenced this pull request Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants