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 support for transport plugins #183

Closed
wants to merge 5 commits into from

Conversation

frekw
Copy link

@frekw frekw commented Jan 22, 2014

This patch introduces support for registering additional transport plugins, thus making it possible to override how Raven pushes data into Sentry.

New transports are added using Raven.registerTransport(protocol, handler). where handler is an object implementing the signature { setup: function(dsn, triggerEvent), send: function(data, endpoint) }.

What transport is used is figured out by parsing the DSN passed into Raven.config.

This patch also introduces a V8Transport which uses CORS to push data into Raven via XHR.

Further discussion can be found here: #157

@mattrobenolt
Copy link
Contributor

@frekw This is awesome! I'm going to want to look at this over the weekend then I'll give my feedback. 👍

@drd
Copy link

drd commented Mar 12, 2014

Any news on this patch? I was just about to hack raven so I could proxy requests to our sentry server and it looks like a simple modification of the default transport plugin here would serve me well.

kgritesh pushed a commit to kgritesh/raven-js that referenced this pull request Jun 4, 2014
@cellofellow
Copy link

Don't want to complain but I need this patch.

@tom-parker
Copy link

Is this likely to be dropping anytime soon?

@raimo
Copy link

raimo commented Sep 5, 2014

+1

@gmathieu
Copy link

Anything we can do to keep this moving?

@mattrobenolt
Copy link
Contributor

@gmathieu Test it out. :)

return HTTPGetTransport;
}

if(!dsn.pass)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend also checking if the user wants https here so they could also use http+post

if(!dsn.pass && dsn.protocol == 'https+post')

@mattrobenolt
Copy link
Contributor

I'm gonna hack on this tonight fwiw.


;(function(window, Raven, undefined){
'use strict';
var V8Transport = window.V8Transport = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call this one XHRTransport?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because sadly, this doesn't just work everywhere that XHR is accepted, so this is specific to V8.

@dcramer
Copy link
Member

dcramer commented Mar 27, 2015

As this is coming up more and more, I think we're going to need to push this through.

One point of feedback I'd like to make: it's probably worthwhile to drop our DSN "https+foo" notation in JavaScript. I think from a usability standpoint its great, but in JavaScript where minifiers are still king its wasteful (as it cant be optimized out).

What if instead we did this:

var legacyTransport = require('raven/transports/legacy')

Raven.config('...', {
  transport: legacyTransport
});

Again, ideally here the minifiers have enough information to optimize out the unused code (transports) and then we could bake in both (CORs and Legacy) by default for our distributed JS bundle.

(I'm open to alternatives here, I just want to make sure the systems can optimize out unused code, and we still maintain it well enough to use both the current transport as well as the new cors stuff)

@dcramer
Copy link
Member

dcramer commented Mar 29, 2015

Here's what I propose, built on this existing PR:

  • Swap out DSN registration for simple transport declaration
  • Replace v8-xhr with transport that works in all modern browsers using XHR+Cors
  • Add a jQuery transport which isn't bundled by default

@realityking
Copy link

@dcramer What advantage would a jQuery transport bring over plain XHR?

@dcramer
Copy link
Member

dcramer commented Mar 29, 2015

@realityking just less code. It may not be relevant, but I imagine the XHR transport having an excess of duplicated code.

@realityking
Copy link

@dcramer I may be a bit slow right now (it's late...) but wouldn't any code size benefit require, that the XHR transport isn't bundled by default either?

@dcramer
Copy link
Member

dcramer commented Mar 29, 2015

@realityking thats correct. The goal is to optimize around the patterns that are becoming more common: requiring modules vs some offline bundler (require.js, common.js, webpack). In that case we could default the transport but allow them to swap it out, which should allow deduplication tools to remove the module.

@realityking
Copy link

@dcramer Gotcha, I had forgotten your comment from 2 days ago. Thank's the explanation :)

@elicwhite
Copy link
Contributor

We would really like to see support for custom transport. We are using a proxy to pass these requests through to Sentry and don't want to support the DSN scheme in our proxy.

@ksikka
Copy link

ksikka commented Aug 19, 2015

What's the security risk - if any - of putting the sentry_secret in the DSN?

@dcramer
Copy link
Member

dcramer commented Aug 19, 2015

@ksikka the secret removes the referrer check entirely, thus making it a very easy abuse vector

@ksikka
Copy link

ksikka commented Aug 19, 2015

@dcramer can we fix this by changing the sentry API logic to allow SSL AJAX POST requests without a secret but with an origin/referrer check?

@dcramer
Copy link
Member

dcramer commented Aug 19, 2015

@ksikka I'm not sure what you mean. All public key requests already look for the origin/referer (no matter the scheme). You can use '*' as the allowed domain and it will bypass the actual match.

@ksikka
Copy link

ksikka commented Aug 19, 2015

@dcramer When the method is POST, we're in this else branch:
https://github.com/getsentry/sentry/blob/53fd3882707e6510c8980b1c92f37b01a71afcc1/src/sentry/web/api.py#L227
We assume that the API request is a server-side request, so a sentry_secret is required.

I'm asking, should we change the API code so that if it's an SSL AJAX POST, the API does not require sentry_secret?

@dcramer
Copy link
Member

dcramer commented Aug 19, 2015

@ksikka ah! yeah we would definitely need to fix that upstream

@numaanashraf
Copy link

@dcramer @mattrobenolt Any updates on this?

@benvinegar
Copy link
Contributor

@numaanashraf – I think this patch is still in limbo and will likely end up closed. In the meantime, if you need to override the default request creator, you can do so by specifying a transport option in your Raven config.

https://github.com/getsentry/raven-js/blob/master/src/raven.js#L748

@mattrobenolt
Copy link
Contributor

In the meantime, if you need to override the default request creator, you can do so by specifying a transport option in your Raven config.

Also, to clarify, this is undocumented for a reason right now. 😉 This is likely the approach we're going, but we're not committing to it just yet.

@benvinegar
Copy link
Contributor

Documentation for the transport config option is available here.

@mattrobenolt
Copy link
Contributor

I'm closing this now in favor of our approach that made it into 1.2.0.

Thanks everybody. :)

kamilogorek pushed a commit that referenced this pull request Jun 12, 2018
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.