feat: Support modifying attachments after init (continued)

[jpnurmi]

TODO:

• Rebase starting point by @Swatinem
  • feat: Support modifying attachments after init #433
• Crashpad IPC (Windows & Linux)
  • feat: add CrashpadClient::Add/RemoveAttachment() for Windows and Linux crashpad#124
• Add _n API
  • feat: extend API with ptr/len-string interfaces #827
• Add attachment API for local scopes
  • feat: Add support for capturing events with local scopes #1248
• Merge & apply scope attachments to envelopes
• Add unit & integration tests
• Update docs
  • feat(native): support modifying attachments after init sentry-docs#14019

See: #586

[github-actions]

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

Generated by 🚫 dangerJS against 05b138f

[jpnurmi]

Hmm, is there a potential need for the user to be able to specify attachment or content types? Instead of using path strings in the API, an opaque sentry_attachment_t type would allow for a nice extension of the attachment API later, including memory buffers.

sentry_attachment_t* file_attachment = sentry_attachment_from_path(png_path);
sentry_attachment_set_content_type(file_attachment, "image/png");
sentry_add_attachment(file_attachment);

sentry_attachment_t* memory_attachment = sentry_attachment_from_buffer(png_data, png_data_len);
sentry_attachment_set_content_type(memory_attachment, "image/png");
sentry_add_attachment(memory_attachment);

[supervacuus]

This looks great. Already approved the crashpad changes.

This also provides a solution for #978 if one uses a local scope together with the dynamic attachment. Adding this use case to the documentation might be a good idea.

[supervacuus]

Hmm, is there a potential need for the user to be able to specify attachment or content types?

We recently discussed this in the context of view hierarchies. There is no sensible reason to expose the attachment type, since those should be handled by the SDK internally.

Content type could be something to expose if users use non-standard extensions that the Python MIME type checker doesn't recognize.

Instead of using path strings in the API, an opaque sentry_attachment_t type would allow for a nice extension of the attachment API later, including memory buffers.

I agree that using short-lived (eaten by "add") opaque attachments could be a nice addition. However, would we also provide option interfaces for those? Should we still offer the less verbose path-based variants?

[jpnurmi]

Instead of using path strings in the API, an opaque sentry_attachment_t type would allow for a nice extension of the attachment API later, including memory buffers.

I agree that using short-lived (eaten by "add") opaque attachments could be a nice addition. However, would we also provide option interfaces for those? Should we still offer the less verbose path-based variants?

I was worried about introducing API that would steal the best names without being easy to extend, but it would be unfortunate to lose the convenient path-based API. If attachment-specific API is ever needed, changing sentry_add_attachment to return the attachment could be a nice way to retain that convenience.

[supervacuus]

I was worried about introducing API that would steal the best names without being easy to extend, but it would be unfortunate to lose the convenient path-based API. If attachment-specific API is ever needed, changing sentry_add_attachment to return the attachment could be a nice way to retain that convenience.

I understand that worry. I think if you want to future-proof this, then we could decide on the following naming:

• sentry_add_attachment_path (or sentry_attachment_add_path, since we often use sentry_module_verb(_thing), but in this case arguably we could provide these for constructing the attachment) for the API that you expose in this PR
• sentry_add_attachment_buffer (or sentry_attachment_add_buffer) for the API that reads an in-memory buffer
• sentry_add_attachment (or sentry_attachment_add) for the generic thing that constructs an attachment object around an opaque handle (which the other two use internally)

With the naming set in stone, you could now release sentry_add_attachment_path first without killing the option of introducing the other interface names. There is no breakage concerning the limited path interface (which reflects the current option API) if we decide to add the others later. Of course, if you want to provide the generic one immediately, that is fine as well, but it's certainly not a blocker for me.

[jpnurmi]

I checked sentry-docs and other SDKs for inspiration. It appears that in-memory buffers are generally referred to as "data" and/or "bytes".

If we reserve sentry_add_attachment(obj) for the object-based API, how do you like:

1. sentry_add_attachment_from_file(path) vs. sentry_add_attachment_from_data(bytes, len), or
2. sentry_attach_file(path) vs. sentry_attach_data(bytes, len)

for the path and data-based convenience APIs?

Expand here to see the vision in more detail...

Current API

For reference. Could be deprecated and removed in 1.0 in favor of the new global APIs.

Options

void sentry_options_add_attachment(sentry_options_t *opts, const char *path);
void sentry_options_add_attachment_n(sentry_options_t *opts, const char *path, size_t path_len);
void sentry_options_add_attachmentw(sentry_options_t *opts, const wchar_t *path);
void sentry_options_add_attachmentw_n(sentry_options_t *opts, const wchar_t *path, size_t path_len);

Proposed API

Here's a draft of the attachment API with some potential future additions.

Global

// later
// void sentry_add_attachment(sentry_attachment_t *attachment);

// alternative names: sentry_attach_file, sentry_add_attachment_path
void sentry_add_attachment_from_file(const char *path);
void sentry_add_attachment_from_file_n(const char *path, size_t path_len);
void sentry_add_attachment_from_filew(const wchar_t *path);
void sentry_add_attachment_from_filew_n(const wchar_t *path, size_t path_len);

// later (alternative names: sentry_attach_data, sentry_add_attachment_data)
// void sentry_add_attachment_from_data(const uint8_t *bytes, size_t bytes_len, const char *filename); // + _n + w

void sentry_remove_attachment(const char *path); // or filename for in-memory buffers
void sentry_remove_attachment_n(const char *path, size_t path_len);
void sentry_remove_attachmentw(const wchar_t *path);
void sentry_remove_attachmentw_n(const wchar_t *path, size_t path_len);

Scope

// later
// void sentry_scope_add_attachment(sentry_scope_t *scope, sentry_attachment_t *attachment);

// alternative names: sentry_scope_attach_file, sentry_scope_add_attachment_path
void sentry_scope_add_attachment_from_file(sentry_scope_t *scope, const char *path);
void sentry_scope_add_attachment_from_file_n(sentry_scope_t *scope, const char *path, size_t path_len);
void sentry_scope_add_attachment_from_filew(sentry_scope_t *scope, const wchar_t *path);
void sentry_scope_add_attachment_from_filew_n(sentry_scope_t *scope, const wchar_t *path, size_t path_len);

// later (alternative names: sentry_scope_attach_data, sentry_scope_add_attachment_from_data)
// void sentry_scope_add_attachment_data(sentry_scope_t *scope, const uint8_t *bytes, size_t bytes_len, const char *filename); + _n + w

Attachment (later)

// alternative names: sentry_attachment_new_path
sentry_attachment_t *sentry_attachment_from_file(const char *path); // + _n + w

// alternative names: sentry_attachment_new_data
sentry_attachment_t *sentry_attachment_from_data(const uint8_t *bytes, size_t bytes_len, const char *filename); // + _n + w

// void sentry_attachment_set_content_type(sentry_attachment_t *attachment, const char *content_type);
// void sentry_attachment_set_filename(sentry_attachment_t *attachment, const char *filename);  // + _n + w
// ...

[supervacuus]

I checked sentry-docs and other SDKs for inspiration. It appears that in-memory buffers are generally referred to as "data" and/or "bytes".

If we reserve sentry_add_attachment(obj) for the object-based API, how do you like:

1. sentry_add_attachment_from_file(path) vs. sentry_add_attachment_from_data(bytes, len), or
2. sentry_attach_file(path) vs. sentry_attach_data(bytes, len)

for the path and data-based convenience APIs?

I fully agree with the proposed structure; however, I think I would still follow the current naming scheme:

• sentry_attachment_add(obj)
• sentry_attachment_add_from_file(path)
• sentry_attachment_add_from_data(buffer)
• sentry_attachment_remove* (see below)
• sentry_attachment_add_to_scope(obj, scope)
• sentry_attachment_add_from_*_to_scope([path|buffer], scope)

This groups all attachment functions when sorted lexicographically.

Only with

void sentry_remove_attachment(const char *path); // or filename for in-memory buffers

It became clear to me that we need a reference here for non-path-based removal... (you probably don't want to compare buffers byte by byte for removal). Even buffer-based attachments require a filename to be visible in the UI. Meaning that the interface will be the same for any type of attachment source (which is probably what you meant anyway, but just to be sure).

[jpnurmi]

The proposal was meant to follow the sentry(_module)_verb(_thing) naming scheme, but it indeed depends on how things should be organized. It seems natural to let scopes, options, and envelopes know how to add attachments. It feels backward to let an attachment know how to add itself to scopes, options, and envelopes. 🙂 I would propose reserving sentry_attachment_xxx for API that operates on a specific attachment object.

• sentry_options_add_attachment operates on options (first "this" argument)
• sentry_add/remove_attachment operates on the global scope (no "this" argument, implicit)
• sentry_scope_add_attachment operates on a local scope (first "this" argument)
• sentry_attachment_xxx operates on a specific attachment object (first "this" argument)

[supervacuus]

That is a sensible argument, and I largely agree (especially wrt sentry_[options|scope|attachment]_*) as stated in my initial response from yesterday).

My only concern is that the proposed global scope operations clutter the name hierarchy with verbs that diverge significantly in their utility and fail to name what they do (not the fault of your proposal). However, they are consistent with the current naming (i.e., more complex forms of sentry_set_*) and as such, fine.

Thanks for the naming iteration. This is time well spent when considering the addition of interfaces later on.

[supervacuus]

My only concern is that the proposed global scope operations clutter the name hierarchy with verbs that diverge significantly in their utility and fail to name what they do (not the fault of your proposal). However, they are consistent with the current naming (i.e., more complex forms of sentry_set_*) and as such, fine.

In this light, I would probably opt for the shorter (global/local) scope function names in your proposal. Meaning

• sentry_attach_file* instead of sentry_add_attachment_from_file*
• sentry_scope_attach_file* instead of sentry_scope_add_attachment_from_file*

[supervacuus]

Beauty, thx! That got quite a bit of polish during the last rounds. I agree that using the actual opaque pointer as a handle to refer to attachments is better than going via the filename.

From my side this is ready.

[supervacuus]

Beauty, thx! That got quite a bit of polish during the last rounds. I agree that using the actual opaque pointer as a handle to refer to attachments is better than going via the filename.

From my side this is ready.

[supervacuus]

Beauty, thx! That got quite a bit of polish during the last rounds. I agree that using the actual opaque pointer as a handle to refer to attachments is better than going via the filename.

From my side this is ready.

[supervacuus]

Nothing is worth being said if you can't do it at least three times. 😅 I guess yesterday's outage is still ongoing.

[jpnurmi]

Thanks for the feedback and guidance! I'm also happy with the result :)

This also provides a solution for #978 if one uses a local scope together with the dynamic attachment. Adding this use case to the documentation might be a good idea.

I opened a doc PR here:

• feat(native): support modifying attachments after init sentry-docs#14019