feat: programatic minidump capture #1052

PlasmaDev5 · 2024-10-11T20:01:37Z

Resolves: #1050

Summary

Provides a new function that allows sentry to capture user created minidumps.

This provides a new function that will allow for independently created minidumps to be captured by sentry Resolves: #1050

github-actions · 2024-10-11T20:02:02Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 67bd3af

vaind

Thanks for the change. looks straightforard so far.

Is this PR done and ready for review? because it's marked as such, I'll asume so (if not, you ca use draft PRs in the future so that reviewers don't get notifications yet).

CI is currently failing so that will need to be fixed. Also, the newly added API should have a new test case.

include/sentry.h

PlasmaDev5 · 2024-10-12T00:44:27Z

Thanks for the change. looks straightforard so far.

Is this PR done and ready for review? because it's marked as such, I'll asume so (if not, you ca use draft PRs in the future so that reviewers don't get notifications yet).

CI is currently failing so that will need to be fixed. Also, the newly added API should have a new test case.

i guess somewhere between draft and final. I for sure not indented to be be merged yet but at the same time i need feedback and review before moving further.

include/sentry.h

supervacuus

Thanks @PlasmaDev5!

There are still quite a few open design decisions to consider. In this case, I tend to leave the duplication because there are already visible diverging needs.

I know that you haven't finished this yet, but the function needs

clear header documentation of tbd design decisions mentioned in my comments
an integration test and an example.c entry

include/sentry.h

src/sentry_core.c

PlasmaDev5 · 2024-10-14T22:56:45Z

Feel i addressed feedback provided.
Unit test and mentioned example.c still need doing tomorrow. but wanted to get a little more feedback

codecov · 2024-10-14T23:37:43Z

Codecov Report

Attention: Patch coverage is 9.52381% with 38 lines in your changes missing coverage. Please review.

Project coverage is 81.25%. Comparing base (21c6d0a) to head (8233951).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1052      +/-   ##
==========================================
- Coverage   81.81%   81.25%   -0.57%     
==========================================
  Files          53       53              
  Lines        6363     6402      +39     
  Branches     1207     1214       +7     
==========================================
- Hits         5206     5202       -4     
- Misses       1045     1086      +41     
- Partials      112      114       +2

supervacuus

Thanks, things are getting in shape.

src/sentry_core.c

PlasmaDev5 · 2024-10-15T21:20:54Z

Addressed review feedback.
Note: feedback to split declaration and initialization of envelope creates a lint error.

I also have work towards the test case and will post a draft version in a comment on here once i start work tomorrow for feedback as i am a lot more unsure with how we want to handle this step.

supervacuus

If the format linter is bugging you, the Native SDK has a .clang-format spec that you can use locally.

If you have any questions about integration testing, please let me know.

src/sentry_core.c

PlasmaDev5 · 2024-10-16T19:00:21Z

To give a rundown on what i have planned for the tests

I am referencing the attachment tests.
It essentially comes down to using sentry__path_write_buffer, from what i can tell it should be able to create a .dmp file and just add some unused data. I can then use the new API to send capture this file.
Im not sure if there is a better solution as im still getting to grips with the available API.

PlasmaDev5 · 2024-10-17T18:58:54Z

@supervacuus Any feedback on the test case stuff? and hopefully all review feedback is addressed.

src/sentry_core.c

supervacuus

I am referencing the attachment tests.
It essentially comes down to using sentry__path_write_buffer, from what i can tell it should be able to create a .dmp file and just add some unused data. I can then use the new API to send capture this file.
Im not sure if there is a better solution as im still getting to grips with the available API.

Given that the inner parts of sentry_capture_minidump() are already covered by unit tests and its side effects primarily affect the network and the file system, I think a basic integration test should suffice for starters.

What I would do is the following:

Build and run the sentry_example with log and crash arguments to produce a minidump (which you can find in the database directory after the crash) and store it in tests/fixtures. At this point, you can also upload the debug info files, which will be useful if you want to verify what the function uploaded in the WebUI.
Add a new command (capture-minidump) to example.c that passes that fixture as an (initially hard-coded absolute system-) path and an empty event. This also lets you experiment manually with the function to see what the WebUI renders for the incoming event.
Alternatively, you can also send any file as a first step since we don't check whether the path pointed to contains a valid minidump
Get an integration test running that invokes the example using your new capture-minidump command (take inspiration from test_capture_http in test_integration_http.py, removing anything relating to transport-compression, but adding an assert_minidump(envelope) at the end)
You can run integration tests locally via make tests, scripts\run_tests.ps1 if you are on Windows or directly via pytest. You can check out the Makefile or the Powershell script to see the venv setup if you want to run this manually.
to allow the test to run in CI, you must pass the fixture path from the test to the example executable (either via command-line arg or via the environment) or let the test or build script copy the fixture to a known location relative to the example executable (which might be helpful to test the removal case).
if the above works,
- add a test case for (other) attachments (using the command attachment + adding assert_attachment(envelope) to the case)
- Add a test case for capture-minidump-remove to verify that the file at the input path was removed.

supervacuus · 2024-10-18T08:34:45Z

src/sentry_core.c

+        if (!envelope || !sentry__envelope_add_event(envelope, event)) {
+            sentry_envelope_free(envelope);
+            sentry_value_decref(event);
+        }


This block must return and free the dump path.

supervacuus · 2024-10-18T08:36:50Z

src/backends/sentry_backend_breakpad.cpp

-    SENTRY_WITH_OPTIONS (options) {
+     SENTRY_WITH_OPTIONS (options) {


Nit: unnecessary whitespace.

vaind · 2024-10-31T16:41:27Z

src/sentry_core.c

+        if (remove_dump_on_send) {
+            sentry__path_remove(sentry_dump_path);
+        }


Because this parameter doesn't actually do what it says in the name (and it cannot at the moment), I'd remove it completely and let users move/remove the file after this function returns.

Note: I could of course remove it myself since I'm taking over this PR, just raised this as a discussion in the PR because I know the original issue had a request to remove processed files. It just doesn't fit with the rest of the API in my opinion.

bruno-garcia · 2024-11-01T14:33:31Z

Closing in favor of:

feat: expose function to send minidumps #1067

PlasmaDev5 and others added 2 commits October 11, 2024 19:57

feat: programmatic minidump capture

d6e922d

This provides a new function that will allow for independently created minidumps to be captured by sentry Resolves: #1050

fixed compile errors

a84fd99

PlasmaDev5 requested review from supervacuus and vaind October 11, 2024 20:01

vaind requested changes Oct 11, 2024

View reviewed changes

include/sentry.h Outdated Show resolved Hide resolved

include/sentry.h Outdated Show resolved Hide resolved

ShawnCZek reviewed Oct 12, 2024

View reviewed changes

include/sentry.h Outdated Show resolved Hide resolved

bruno-garcia changed the title ~~Feat/programatic minidump capture~~ feat: programatic minidump capture Oct 14, 2024

supervacuus requested changes Oct 14, 2024

View reviewed changes

include/sentry.h Outdated Show resolved Hide resolved

include/sentry.h Outdated Show resolved Hide resolved

include/sentry.h Outdated Show resolved Hide resolved

include/sentry.h Outdated Show resolved Hide resolved

src/sentry_core.c Outdated Show resolved Hide resolved

PlasmaDev5 added 2 commits October 14, 2024 23:41

Rework of programmatic minidumps based on feedback

2a3c943

Remove unused parameters

081a05c

Address Lint errors

c141201

supervacuus requested changes Oct 15, 2024

View reviewed changes

PlasmaDev5 added 3 commits October 15, 2024 22:10

Address more review feedback

ac83d5b

Address Lint errors

39c0bfd

Address more lint errors

6b25152

changed from bool to int due to undefined errors

1f42aaa

supervacuus requested changes Oct 16, 2024

View reviewed changes

src/sentry_core.c Outdated Show resolved Hide resolved

src/sentry_core.c Outdated Show resolved Hide resolved

Address review feedback

8233951

supervacuus reviewed Oct 18, 2024

View reviewed changes

src/sentry_core.c Outdated Show resolved Hide resolved

supervacuus requested changes Oct 18, 2024

View reviewed changes

PlasmaDev5 added 3 commits October 23, 2024 19:02

Work on addressing feedback, example and test

ed51a88

Fixes to lint errors

93b2984

Apply comment suggestion

293a353

PlasmaDev5 and others added 5 commits October 23, 2024 20:26

Add empty line to file end

5878abc

use parameter directly

6170d7d

Merge branch 'master' into feat/programatic_minidump_capture

f240fbb

chore: fix changelog

0f1c8b6

add capture_minidump_n

67bd3af

vaind reviewed Oct 31, 2024

View reviewed changes

vaind mentioned this pull request Oct 31, 2024

feat: expose function to send minidumps #1067

Merged

bruno-garcia closed this Nov 1, 2024

bruno-garcia deleted the feat/programatic_minidump_capture branch November 1, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: programatic minidump capture #1052

feat: programatic minidump capture #1052

PlasmaDev5 commented Oct 11, 2024

github-actions bot commented Oct 11, 2024 •

edited

Loading

vaind left a comment

PlasmaDev5 commented Oct 12, 2024

supervacuus left a comment

PlasmaDev5 commented Oct 14, 2024

codecov bot commented Oct 14, 2024 •

edited

Loading

supervacuus left a comment

PlasmaDev5 commented Oct 15, 2024

supervacuus left a comment

PlasmaDev5 commented Oct 16, 2024

PlasmaDev5 commented Oct 17, 2024

supervacuus left a comment •

edited

Loading

supervacuus Oct 18, 2024

supervacuus Oct 18, 2024

vaind Oct 31, 2024 •

edited

Loading

vaind Oct 31, 2024

bruno-garcia commented Nov 1, 2024

		SENTRY_WITH_OPTIONS (options) {
		SENTRY_WITH_OPTIONS (options) {

feat: programatic minidump capture #1052

feat: programatic minidump capture #1052

Conversation

PlasmaDev5 commented Oct 11, 2024

Summary

github-actions bot commented Oct 11, 2024 • edited Loading

vaind left a comment

Choose a reason for hiding this comment

PlasmaDev5 commented Oct 12, 2024

supervacuus left a comment

Choose a reason for hiding this comment

PlasmaDev5 commented Oct 14, 2024

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

supervacuus left a comment

Choose a reason for hiding this comment

PlasmaDev5 commented Oct 15, 2024

supervacuus left a comment

Choose a reason for hiding this comment

PlasmaDev5 commented Oct 16, 2024

PlasmaDev5 commented Oct 17, 2024

supervacuus left a comment • edited Loading

Choose a reason for hiding this comment

supervacuus Oct 18, 2024

Choose a reason for hiding this comment

supervacuus Oct 18, 2024

Choose a reason for hiding this comment

vaind Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

vaind Oct 31, 2024

Choose a reason for hiding this comment

bruno-garcia commented Nov 1, 2024

github-actions bot commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

supervacuus left a comment •

edited

Loading

vaind Oct 31, 2024 •

edited

Loading