- Notifications
You must be signed in to change notification settings - Fork 569
feat(attachments): Add basic support for attachments #856
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
philipphofmann left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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