-
-
Notifications
You must be signed in to change notification settings - Fork 135
Conversation
If you could add a cursory test for this I will merge pronto. |
@benvinegar can do! |
@@ -41,6 +41,7 @@ var Client = function Client(dsn, options) { | |||
this.root = options.root || process.cwd(); | |||
this.transport = options.transport || transports[this.dsn.protocol]; | |||
this.release = options.release || process.env.SENTRY_RELEASE || ''; | |||
this.environment = options.environment || process.env.SENTRY_ENVIRONMENT || ''; |
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.
Wouldn't we want to leverage NODE_ENV
for this? Or is that not really a thing anymore?
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 kind of agree, and was going to ping @dcramer whether SENTRY_ENVIRONMENT was a thing.
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.
@mattrobenolt @benvinegar I thought about that but saw from the blog:
By default we automatically select the most production like environment
and I also saw this so I opted not too. I'm happy to add it in as a fallback!
fwiw, most people I know run node in 'production' for staging sites so I don't know how much value it would add
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.
Remove NODE_ENV entirely, fixes many issues since people have different opinions on wtf this means
lol, yeah, I remember this. Before, it was automatically enabling/disabling Raven as a whole if NODE_ENV
wasn't production.
But yeah, you might be right. I don't think this is really used to mean an environment. I don't think someoen would say NODE_ENV=staging
, etc.
@benvinegar tests added 👍 |
Fantastic. Thanks! |
Did anybody "functionally tested" the thing because I cannot see the I'm using |
Well, reading the Sentry documentation you can set the
What I'm saying is that, setting the environment worked prior to
And it works, I get the |
Hmm, yeah, looking back ... not sure why I thought this was functionally correct. I'll follow-up with a fix.
The environment tag is different from the explicit "environment" attribute. More here: https://docs.getsentry.com/hosted/clientdev/attributes/ |
See #185 |
It works now with #185. But Sentry is quite weird with the |
If I had to guess I'd say you're sending "Environment" and "environment" and at this time tags are case sensitive. |
Alternatively it might be that it's tagging it twice but most of our code doesn't support that. Either way you don't need to send the tag when you've sent the top level attribute. |
Don't worry, I used the very same strings ;) Ok then, now waiting for 0.23.0 I guess. |
In order to support https://blog.getsentry.com/2016/07/22/environment-details.html
This change is