Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add a "silent" option for test/development #13

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

bcherry commented Jul 31, 2012

This change adds a simple "silent" mode, as described by Issue #12 from @zzen.

You simply pass { silent: true } into the Client constructor, and it will swallow two things: The failure to find/parse the DSN (because you have no DSN), and the failure to run in NODE_ENV='production'. I'm using a dummy dsn object to avoid later failures down the pipe (since captureMessage, for instance, returns an object even if it's disabled and I didn't want to change that).

In practice, just add the { silent: true } flag if you're using Raven in a development server or test harness and don't want to see extra log messages appear, and don't want to add extra logic and branching to avoid using it.

Might not be the cleanest implementation, but it gets the job done.

Owner

mattrobenolt commented Jul 31, 2012

I'm down with the { silent: true } option, but not sure why you'd want to suppress the invalid DSN. That seems a bit of a stretch for me.

I think overall, the better approach to this might be just an environment variable to disable Raven entirely. That way you can just avoid it all together during testing, since that seems more like the issue at hand. Something like: RAVEN_ENABLED=0

/cc @zzen

Owner

mattrobenolt commented Jul 31, 2012

After talking with @dcramer, we've come to the same conclusion:

A falsey DSN value should be a flag for "disabled".

I dislike the idea of running { silent: true } in your code that's being tested for one reason: the code being tested is what should be shipped. And shipping something with an invalid DSN could be an issue and should at least raise a warning, so I'd discourage against setting it at all, which would negate the entire point.

I think the issue of silencing Raven yelling at you could be averted by an environment variable to disable raven all together that gets applied to your testing tool, ala RAVEN_ENABLED=0 make test or supply a falsey value for your DNS and just get a simple warning instead of an Error being raised.

zzen commented Jul 31, 2012

Agreed. After much hair-pulling we wrapped raven in our own logging class that disables Raven initialisation in development code.

Running with production DSN in TDD unit testing on local developer machines is not feasible as you can surely see...

bcherry commented Jul 31, 2012

My local and test environments have no DSN, and it would be a bad idea to include a real DSN there. Currently, I'm faking a DSN like 'http://foo:bar@localhost', but that's not really a good idea either. Falsey DSN as a trigger sounds good to me, since it's the most obvious thing you could do. Still, I'd really prefer if it didn't log at all, so it doesn't appear in command-line testing output and look like an error. That's just a preference, and it's not a huge deal. The environment variable sounds like a decent, if heavy-handed, approach, but it would be much cleaner as opt-out rather than opt-in (as in RAVEN_DISABLED=1 rather than RAVEN_ENABLED=0).

Or, alternatively, supplying a raven.DummyClient() or disabled flag in the Client constructor that makes a fake/disabled client and ignores DSN would be nice (as @zzen has done on his own). I'm comfortable with a single switch in the my init code that puts a disabled class in the way, I'd rather not have to try to mirror the API on my own though.

Owner

mattrobenolt commented Jul 31, 2012

Is the DSN declared in your actual Javascript? Or are you using the SENTRY_DSN env variable?

I'm just trying to gather information about how it's actually being used. The problem with Node, in this case, is lack of conformity. It seems that every project is built/run differently, so I'd like to cover most use cases without dictating how things should be done.

zzen commented Jul 31, 2012

For us the DSN is in environment variable. If the variable is empty we provide a fake DSN default in the code.

On 31. 7. 2012, at 21:27, Matt Robenoltreply@reply.github.com wrote:

Is the DSN declared in your actual Javascript? Or are you using the SENTRY_DSN env variable?

I'm just trying to gather information about how it's actually being used. The problem with Node, in this case, is lack of conformity. It seems that every project is built/run differently, so I'd like to cover most use cases without dictating how things should be done.


Reply to this email directly or view it on GitHub:
mattrobenolt#13 (comment)

Owner

mattrobenolt commented Jul 31, 2012

@zzen I like that approach. I think I'm going to put that into Raven as default, so an empty DSN is implicitly disabling Sentry. It seems like a logical assumption.

An invalid DSN, should still, in my opinion, raise an exception. That would be the case of a malformed DSN.

zzen commented Jul 31, 2012

Matt, that would certainly work for me...

On 31. 7. 2012, at 21:54, Matt Robenoltreply@reply.github.com wrote:

@zzen I like that approach. I think I'm going to put that into Raven as default, so an empty DSN is implicitly disabling Sentry. It seems like a logical assumption.

An invalid DSN, should still, in my opinion, raise an exception. That would be the case of a malformed DSN.


Reply to this email directly or view it on GitHub:
mattrobenolt#13 (comment)

bcherry commented Jul 31, 2012

Same for me (env variable, fake one generated in code if it's not present). Your solution sounds correct to me, so I'm going to close this pull request. Thanks for looking into this!

@bcherry bcherry closed this Jul 31, 2012

zzen commented Aug 8, 2012

I just checked latest Raven and I'm still unsure of the result of this thread.

  • Are we still on schedule for "an empty DSN is implicitly disabling Sentry"?
  • Should we perhaps leave this open until that happens as a tracking?
Owner

mattrobenolt commented Aug 10, 2012

@zzen That is the conclusion and resolution that @zeeg and I came up with. I just haven't implemented just yet. I'll make some time tonight to slap it in. I'll create a new issue for it specifically.

@mattrobenolt mattrobenolt referenced this pull request Aug 11, 2012

Merged

Tell Sentry to shut up #15

@mattrobenolt mattrobenolt added a commit that referenced this pull request Aug 14, 2012

@mattrobenolt mattrobenolt Merge pull request #15 from mattrobenolt/silent
Tell Sentry to shut up, fixes #13 #14
7b93584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment