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

Add endpoint to log client events #5918

Merged
merged 19 commits into from Nov 8, 2021
Merged

Conversation

crobibero
Copy link
Member

@crobibero crobibero commented Apr 26, 2021

Fixes #5646

This logs all events to files separated by the ClientName parameter as the following to the file log_{ClientName}_{yyyymmdd}.log

[2021-04-26 12:21:00.108 +00:00] [Trace] [Jellyfin-Sample-Client:1.0.0]: UserId: 0000000-0000-0000-0000-000000000000 DeviceId:  8eec4223-9280-454f-a4ee-bc978c1007e4 
This is the log message 

Updated openapi: openapi_client_event.json.txt

Note- these endpoints don't currently require authentication, so that may change depending on feedback.

@dkanada
Copy link
Member

dkanada commented Apr 26, 2021

Can we put the client name first so they're all in a single location on the filesystem? Our ffmpeg logs have a similar pattern, which I find quite convenient.

@BaronGreenback
Copy link
Contributor

Definitely useful for dlna-playTo client logging.

Copy link
Member

@daullmer daullmer left a comment

Choose a reason for hiding this comment

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

Shouldn't the api endpoint require authorization?

Co-authored-by: dkanada <dkanada@users.noreply.github.com>
@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Aug 30, 2021
@crobibero crobibero closed this Sep 27, 2021
@crobibero crobibero deleted the client-logger branch September 27, 2021 12:55
@crobibero crobibero restored the client-logger branch October 26, 2021 20:08
@crobibero crobibero reopened this Oct 26, 2021
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

Would be nice to have an endpoint to upload a large log for when clients crash and want to upload a dump.

edit: so not an array but really just a plaintext document

@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Oct 27, 2021
@crobibero
Copy link
Member Author

Would be nice to have an endpoint to upload a large log for when clients crash and want to upload a dump.

edit: so not an array but really just a plaintext document

This has been added. The files will automatically be cleaned up by the existing DeleteLogFileTask

@Shadowghost
Copy link
Contributor

I still don't like this approach. The server should not be responsible for proper logging on the clients.

You still need an admin to access the uploaded logs, it's not really an improvement for normal users.

Copy link
Member

@dkanada dkanada left a comment

Choose a reason for hiding this comment

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

Could a setting to globally enable the feature as well as one to set the IClientEventLogger used by the server be added? That might help ease some of the concerns raised.

MediaBrowser.Model/ClientLog/ClientLogEvent.cs Outdated Show resolved Hide resolved
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

  • Should we add a administrator option to disable client log uploads?
    • If yes: How will the API tell us a upload was denied? Do we use a HTTP status code for that?
  • Which clients want to use this API?
    • Do they use it for logging, "dumping" or both?
      • ATV will not use the LogEvent(s) operations, only the LogFile operation
  • Would it make sense to rename "LogFile" to something like "LogDocument"?
  • Do we need to limit the size of a log file?
  • Can we get client & device info from the authorization header when logging a file?

Jellyfin.Api/Controllers/ClientLogController.cs Outdated Show resolved Hide resolved
@crobibero
Copy link
Member Author

crobibero commented Oct 28, 2021

System setting added: AllowClientLogUpload -> returns 403 Forbidden if disabled
Max document size: 1M
File is now generated as: upload_$clientName_$clientVersion_$utcNow.log

Co-authored-by: Niels van Velzen <nielsvanvelzen@users.noreply.github.com>
@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Nov 1, 2021
@nielsvanvelzen
Copy link
Member

@crobibero can you rebase to fix the merge conflict and so we can see if #6308 works?

@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Nov 2, 2021
@crobibero
Copy link
Member Author

crobibero commented Nov 3, 2021

@nielsvanvelzen

Error: Resource not accessible by integration
Error: See this action's readme for details about this error

Note: In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions. Private repositories can be configured to enable workflows from forks to run without restriction. See here for further explanation. Alternatively, use the pull_request_target event to comment on pull requests.

@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Nov 3, 2021
@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Nov 3, 2021
[ProducesResponseType(StatusCodes.Status403Forbidden)]
public async Task<ActionResult> LogEvent([FromBody] ClientLogEventDto clientLogEventDto)
{
if (!_serverConfigurationManager.Configuration.AllowClientLogUpload)
Copy link
Member

Choose a reason for hiding this comment

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

Could also have been a policy, but I'm also not a fan of the policies.

@crobibero crobibero requested a review from cvium November 5, 2021 16:41
@nielsvanvelzen
Copy link
Member

@nielsvanvelzen

Error: Resource not accessible by integration
Error: See this action's readme for details about this error

Note: In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions. Private repositories can be configured to enable workflows from forks to run without restriction. See here for further explanation. Alternatively, use the pull_request_target event to comment on pull requests.

This should be fixed now, do you want to rebase again so we can test it?

@crobibero
Copy link
Member Author

@jellyfin-bot rebase

@github-actions
Copy link

github-actions bot commented Nov 6, 2021

Changes in OpenAPI specification found. Expand to see details.

What's New


POST /ClientLog

Post event from client.

POST /ClientLog/Bulk

Bulk post events from client.

POST /ClientLog/Document

Upload a document.

What's Changed


GET /System/Configuration
Return Type:

Changed response : 200 OK

Application configuration returned.

  • Changed content type : application/json

openapi-base openapi-changes.md openapi-head Added property AllowClientLogUpload (boolean)

Gets or sets a value indicating whether clients should be allowed to upload logs.

  • Changed content type : application/json; profile="CamelCase"

openapi-base openapi-changes.md openapi-head Added property AllowClientLogUpload (boolean)

Gets or sets a value indicating whether clients should be allowed to upload logs.

  • Changed content type : application/json; profile="PascalCase"

openapi-base openapi-changes.md openapi-head Added property AllowClientLogUpload (boolean)

Gets or sets a value indicating whether clients should be allowed to upload logs.

POST /System/Configuration
Request:

Changed content type : application/json

Updated ServerConfiguration :

  • Added property AllowClientLogUpload (boolean)

Gets or sets a value indicating whether clients should be allowed to upload logs.

Changed content type : text/json

Updated ServerConfiguration :

  • Added property AllowClientLogUpload (boolean)

Gets or sets a value indicating whether clients should be allowed to upload logs.

Changed content type : application/*+json

Updated ServerConfiguration :

  • Added property AllowClientLogUpload (boolean)

Gets or sets a value indicating whether clients should be allowed to upload logs.

@cvium cvium merged commit 0ee43f8 into jellyfin:master Nov 8, 2021
20 checks passed
@crobibero crobibero deleted the client-logger branch November 8, 2021 13:52
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.

Allow clients to send log messages to server
9 participants