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

Add Raven.setExtraContext and Raven.setTagsContext. #219

Merged
merged 1 commit into from
Jun 25, 2014

Conversation

DouweM
Copy link
Contributor

@DouweM DouweM commented Jun 25, 2014

Resolves #213.

@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

API and semantics (merge rather than overwrite) based on raven-ruby.

@mattrobenolt
Copy link
Contributor

How would you handle the case of unsetting these?

Lemme double check out Python handles this since that's our canonical reference.

@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

@mattrobenolt
Copy link
Contributor

Are you trying to model this after tags_context and extra_context ? If so, this makes sense.

Also, if so, I think I'd prefer to keep naming inline and go with either setTagsContext() or just tagsContext.

@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

Or hmm, it doesn't, it completely overrides the extra/tags, instead of adding onto them. That's different from how the Ruby version behaves.

@mattrobenolt
Copy link
Contributor

Aha!

Yeah, we definitely always merge in python and would prefer to model anything in raven-js after python.

@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

All right, I'll change that then. Annoying how the Ruby version don't adhere to that then :/

I don't mind the renaming, but shouldn't we rename/alias setUser to setUserContext then as well?

@mattrobenolt
Copy link
Contributor

To be fair, we can also rename setUser to setUserContext as well to conform.

@mattrobenolt
Copy link
Contributor

welp

@mattrobenolt
Copy link
Contributor

Yeah, let's do that, rename and leave an alias that I'll deprecate in the future.

@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

All right, changes coming up in a minute, currently trying to whip up a backbone plugin :)

@DouweM DouweM changed the title Add Raven.setExtra and Raven.setTags. Add Raven.setExtraContext and Raven.setTagsContext. Jun 25, 2014
@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

Changes made.

@mattrobenolt
Copy link
Contributor

Awesome! Thanks. 👍

mattrobenolt added a commit that referenced this pull request Jun 25, 2014
Add Raven.setExtraContext and Raven.setTagsContext.
@mattrobenolt mattrobenolt merged commit 5e75116 into getsentry:master Jun 25, 2014
@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

You should ping the Ruby guys their API is nonconforming by the way ;)

@dcramer
Copy link
Member

dcramer commented Jun 25, 2014

The Ruby APIs match the Python APIs which conform to my made up spec. If anything, the JS API is nonconforming :)

@DouweM
Copy link
Contributor Author

DouweM commented Jun 25, 2014

@dcramer Name-wise, yes, but check the source! In the Ruby implementation, new tags are merged with existing tags; in the Python implementation, new tags overwrite existing tags!

@dcramer
Copy link
Member

dcramer commented Jun 26, 2014

Can we add setUser back as well? I wasn't paying much attention to this, but this is actually a pretty big caveat especially if its going to pop up in a minor version release.

@DouweM
Copy link
Contributor Author

DouweM commented Jun 26, 2014

@dcramer
Copy link
Member

dcramer commented Jun 26, 2014

Ah hrm, maybe this is just a documentation issue on our end then. Doh

@dcramer
Copy link
Member

dcramer commented Jun 26, 2014

K addressed in getsentry/sentry@be71a58

We'll want to minor version bump to get the new things first-class I imagine

Thanks!

@DouweM
Copy link
Contributor Author

DouweM commented Jun 26, 2014

This is really something to consider for the canonical Python API, but do we want to be able to not override, but add to the extra attributes?

I'm setting some generic extra attribute at Raven setup:

Raven.config(dsn, {
  extra: {
    request_id: "<%= request.headers["X-Request-ID"] %>"
  }
}).install();

And then later, depending on the exact place Raven is included, I'm setting some more extra attributes that aren't necessarily (known to be) available at config-time:

Raven.setExtraContext(
  {
    accommodation_id:   "<%= current_accommodation.id %>",
    accommodation_host: "<%= current_accommodation.host %>"
  }
);

Right now, the last call overrides the first, meaning request_id is lost, but I'd like the latter to append onto the existing extra attributes. Note that this is already how the Ruby Raven behaves, where I use a very similar scheme of two Raven.extra_context calls.

If we don't want to make this the default, how about adding an extra options object like { merge: true } or just an extra merge boolean arg to the setExtraContext and setTagsContext methods? This would be backward compatible, and could be added to all implementations that currently only do override.

@dcramer
Copy link
Member

dcramer commented Jun 26, 2014

I dont have a strong opinion here. I think two best solutions are:

  1. Initial context is different than local (and it is always merged when event is sent)
  2. We add a merge option

There are likely rare situations where you could have multiple middleware (or similar) and want to set context at different levels, and have that merged in, but I'm not sure if it's worthwhile to cater to that situation.

@DouweM
Copy link
Contributor Author

DouweM commented Jun 26, 2014

It's probably rare in the larger scheme of things, but because of the sort-of layered way our app is built, I would really appreciate a merge option. I also use this feature with the Ruby version, and I'd imagine it'd be useful with Python web frameworks as well.

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.

Feature Request: Allow setting (some) options after initial Raven.config call
3 participants