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

feat: Support modifying attachments after init #433

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Dec 1, 2020

Moves the attachments to the scope, and adds sentry_add_attachment and
sentry_remove_attachment and wstr variants that modify this attachment
list after calling init. Attachments are identified by their path.

Attachments are still based on file paths and loaded lazily, this does not add support for attachments based on in-memory buffers.

Moves the attachments to the scope, and adds `sentry_add_attachment` and
`sentry_remove_attachment` and wstr variants that modify this attachment
list after calling init. Attachments are identified by their path.
@Swatinem Swatinem requested review from philipphofmann and a team December 1, 2020 10:22
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks @Swatinem 💯. The API seems sufficient for synching attachments from Java to native.

@philipphofmann
Copy link
Member

I'm sorry I was wrong. I think we need some more changes to this PR.
In Python and Java, we have two different types of attachments reflected in the same class. An attachment can either have a byte array or a path. Each attachment also has a filename or and a content type, see

In the SDK API Evolution meeting, we decided it is fine to just copy the byte array to native with the tradeoff of increasing the memory footprint.

*
* See the NOTE on attachments above for restrictions of this API.
*/
SENTRY_API void sentry_add_attachment(const char *path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we always want this to be void? Does a status return make any sense now or in the future?

*
* See the NOTE on attachments above for restrictions of this API.
*/
SENTRY_API void sentry_remove_attachment(const char *path);
Copy link
Contributor

Choose a reason for hiding this comment

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

While an idempotent API is good, would a return from which you can tell whether anything happened or not be useful?

* API Users on windows are encouraged to use `sentry_add_attachmentw` instead.
*
* See the NOTE on attachments above for restrictions of this API.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to explicitly say something about the ownership of *path? (same for remove attachment)

@@ -364,7 +364,9 @@ sentry_envelope_serialize(const sentry_envelope_t *envelope, size_t *size_out)

sentry__envelope_serialize_into_stringbuilder(envelope, &sb);

*size_out = sentry__stringbuilder_len(&sb);
if (size_out) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 😄

@getsantry
Copy link

getsantry bot commented Oct 18, 2023

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

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.

3 participants