Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Make ->$foo_context behave more like ->capture_$foo #31

Open
jrubinator opened this issue Apr 7, 2020 · 2 comments
Open

Make ->$foo_context behave more like ->capture_$foo #31

jrubinator opened this issue Apr 7, 2020 · 2 comments

Comments

@jrubinator
Copy link
Contributor

Right now the various ->$foo_context methods (eg. request_context) return only the appropriate context.

Eg. if you do

my %returned_context = Sentry::Raven->request_context(
    %relevant_request_information,
    tags => { foo => 'bar' }
);

then %returned_context will not contain any information about those tags (and there will be no warnings).

This can be surprisingly different than the capture_request call (which does observe non-request context), and besides can lead to slightly over-verbose code like:

    my ($url, %context) = get_sentry_context_from_request($req);
    $sentry->capture_message($message, %context, Sentry::Raven->request_context($url, %context));

(instead of

    my ($url, %context) = get_sentry_context_from_request($req);
    $sentry->capture_message($message, Sentry::Raven->request_context($url, %context));

)

Would you have any interest in a patch to make request_context and company behave more like their capture_ counterparts (and also allow chaining)?

@querry43
Copy link
Contributor

I would be happy to have a patch on this, as long a we maintain backwards compatibility. Thanks!

@jrubinator
Copy link
Contributor Author

@querry43 So I finally took a pass at this, and ultimately decided it's probably not a good idea, at least as specced.

In particularly, user_context needs to allow an arbitrary list of key-value pairs (per https://develop.sentry.dev/sdk/event-payloads/user/). That makes it nigh-impossible to pass in non-user contexts, at least while passing hashes, and it seems a poor idea to have user_context behave differently than its siblings.

However - we could get more invasive while retaining backwards compatibility. If each method's signature preferred hashrefs rather than hashes, eg.

   $raven->capture_exception( $exception_value, \%exception_context, \%general_context )

instead of the current

    $raven->capture_exception( $exception_value, %exception_context, %context )

we could support this in user_context, but retain backwards compatibility (if we don't get a hashref as the first argument, we behave in the current manner).

I'm still game to make that patch, but figured it was worth checking in again before pulling out my scalpel.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants