8000 Initial version for diagnostic logs: by MarcelGeo · Pull Request #452 · MerginMaps/server · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Initial version for diagnostic logs: #452

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 14, 2025
Merged

Initial version for diagnostic logs: #452

merged 5 commits into from
May 14, 2025

Conversation

MarcelGeo
Copy link
Collaborator
@MarcelGeo MarcelGeo commented May 9, 2025 •
edited
Loading

Added ➕

  • add new v2 endpoint to stats module
  • save file to DIAGNOSTIC_LOGS_URL
  • propagate to config

Refactor 🔨

  • moved some endpoints to introduced global controller (latest version)
    Example
marcelkocisek@pop-os:~/Documents/mergin/server$ curl -X POST "http://localhost:5001/v2/diagnostic-logs?app=input-android-0.9.0&username=test-user" -H "Content-Type: text/plain" --data "This is a sample diagnostic log content."
"Log saved successfully"

file is created

image

content

image

❗ ❓

  • some security issues? Do we want to check content or something like this? ✔️
  • enable Auth required (?) 👎

MarcelGeo added 2 commits May 9, 2025 17:00
@MarcelGeo
Initial version for diagnostic logs:
5ba12b0 
- add new v2 endpoint to stats module
- save file to DIAGNOSTIC_LOGS_URL
- propagate to config
@MarcelGeo
fix config getting
0079a0b
@MarcelGeo MarcelGeo requested review from varmar05 and harminius May 9, 2025 15:22
@coveralls
Copy link
coveralls commented May 9, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 15000170926

Details

  • 178 of 183 (97.27%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 91.729%
Changes Missing Coverage Covered Lines Changed/Added Lines %
server/mergin/controller.py 38 43 88.37%
Totals Coverage Status
Change from base Build 14905289570: 0.09%
Covered Lines: 7065
Relevant Lines: 7702
💛 - Coveralls

varmar05
varmar05 requested changes May 12, 2025
View reviewed changes
Copy link
Collaborator
@varmar05 varmar05 left a comment

Choose a reason for hiding this comment

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

I am wondering why we have this in stats module, is not is more generic functionality which can be part of core app?

@MarcelGeo
Refactor:
4c5b6e2 
- move some endpoints to global app.py from stats
- move some tests to `test_controller`
Updated:
- make max size configurable
@MarcelGeo
Copy link
Collaborator Author
MarcelGeo commented May 13, 2025 •
edited
Loading

Under the hood:

  • I made configurable max body size for diagnostic logs DIAGNOSTIC_LOGS_MAX_SIZE
  • global controller created for such endpoints @varmar05 (there some magic with /alive response, therefore I didn't move it to connexion)
  • magic mimetype handling introduced to body

@MarcelGeo MarcelGeo requested a review from varmar05 May 13, 2025 14:28
varmar05
varmar05 approved these changes May 14, 2025
View reviewed changes
Copy link
Collaborator
@varmar05 varmar05 left a comment

Choose a reason for hiding this comment

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

nice one

@MarcelGeo MarcelGeo merged commit 9f864ba into develop May 14, 2025
4 checks passed
@MarcelGeo MarcelGeo deleted the diagnostic-logs-endpoint branch May 14, 2025 06:44
harminius
harminius reviewed May 14, 2025
View reviewed changes
server/mergin/tests/test_controller.py
Comment on lines +21 to +24
print(resp.headers)
print(resp_data)
assert "processing_time_ms" in resp_data
print(type(resp_data))
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarcelGeo rm prints()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harminius harminius harminius left review comments

@varmar05 varmar05 varmar05 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarcelGeo @coveralls @varmar05 @harminius
0