feat(attachments): Add basic support for attachments#856
Conversation
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
| if is_transaction: | ||
| envelope.add_transaction(event_opt) | ||
| else: | ||
| envelope.add_event(event_opt) |
There was a problem hiding this comment.
@rhcarvalho I'm starting to think this entire forcing events and transactions to go different paths on the envelope to be wrong-ish. We now need to check if it's a transaction or not to call a different method on the envelope but in both cases they do the same internally. The only difference is that add_event and add_transaction pass different event types.
philipphofmann
left a comment
There was a problem hiding this comment.
I have no real knowledge about Python, so my comments are mostly about the API and not language details. I didn't expect adding attachments to be so simple 😁
|
Lots of comments, no approvals. Who can sign off on this? |
rhcarvalho
left a comment
There was a problem hiding this comment.
Had an idea to simplify the API, we may be able to reduce the number of input arguments and usage ambiguity.
cc @philipphofmann @bruno-garcia if that idea would translate well for your platforms of interest.
|
@philipphofmann i think this is ready to merge. |
There was a problem hiding this comment.
The only open question for me is why we don't have any overloads for add_attachment and the init of the attachment or maybe I'm missing something.
Either you want to pass bytes for an attachment or a file to read. So two methods would make sense for me. For the first method, you are forced to pass bytes and a filename for the second you only need to pass a path. This forces you to use the API right at compile time and you don't find out at runtime if you passed in the wrong arguments. We could ditch raising TypeError in the init of the Attachment. What do you think about that @mitsuhiko?
def add_attachment(
self,
bytes=None, # type: bytes
filename=None, # type: str
content_type=None, # type: Optional[str]
add_to_transactions=False, # type: bool
)
def add_attachment(
self,
path=None, # type: str
filename=None, # type: Optional[str]
content_type=None, # type: Optional[str]
add_to_transactions=False, # type: bool
)
Other comments are just nitpicking.
|
@philipphofmann there's no such thing as an overload in Python. You can declare an overload in mypy, but that's almost outside of the language. |
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
|
Basically what @untitaker said. This is how people do Python APIs :) |
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Thanks, didn't know that. I never wrote a single line of Python yet 😀 |
Initial work in progress support for attachments.
Example integration that uploads stdout/stderr as attachments is here: https://gist.github.com/mitsuhiko/16815c06341370c90030b520984e6ee2