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

Include custom data in scope sent to rollbar #303

Merged
merged 1 commit into from
Mar 17, 2017

Conversation

mikehale
Copy link

@mikehale mikehale commented Feb 6, 2017

This includes the provided context in the rollbar scope. Previously the context was ignored and not included in the scope. This is similar to how scope is defined for rails in rollbar: https://rollbar.com/docs/notifier/rollbar-gem/#the-scope

@mikehale
Copy link
Author

mikehale commented Feb 6, 2017

Hmm, this does not seem to be working correctly. I'll investigate and update when this is ready.

@mikehale mikehale force-pushed the rollbar-custom-context branch 2 times, most recently from 370e98e to 2bdba4a Compare February 7, 2017 21:05
@mikehale mikehale force-pushed the rollbar-custom-context branch from bb9d2cc to b5df34f Compare March 3, 2017 21:36
@mikehale mikehale changed the title Include context data in scope sent to rollbar Include custom data in scope sent to rollbar Mar 3, 2017
@mikehale mikehale force-pushed the rollbar-custom-context branch from c9b96f6 to b5df34f Compare March 3, 2017 21:48
@mikehale
Copy link
Author

mikehale commented Mar 3, 2017

@gudmundur this is ready for review now.

Copy link
Member

@gudmundur gudmundur left a comment

Choose a reason for hiding this comment

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

Well spotted that we're not passing this on. I've added a couple of minor comments, otherwise LGTM.

.travis.yml Outdated
@@ -9,6 +9,8 @@ before_install:
- gem update --system
before_script:
- createdb pliny-gem-test
before_install:
- gem update --system
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind removing this? This has already been addressed with ae7dc84.

@@ -18,7 +18,7 @@ def notify(exception, context:, rack_env:)
private

def fetch_scope(context:, rack_env:)
scope = {}
scope = { custom: context }
Copy link
Member

Choose a reason for hiding this comment

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

Weakly held opinion, but is there a reason we don't call this context? Does that name hold some significance to Rollbar?

Copy link
Author

Choose a reason for hiding this comment

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

It does hold significance to rollbar. Outside of the request object, they require data be under a custom key. See https://rollbar.com/docs/api/items_post/.

@mikehale mikehale force-pushed the rollbar-custom-context branch from b5df34f to 67403ac Compare March 16, 2017 13:38
@mikehale
Copy link
Author

@gudmundur comments addressed.

@gudmundur gudmundur merged commit 832f2f4 into interagent:master Mar 17, 2017
@gudmundur
Copy link
Member

@mikehale Cheers! 🍻

@mikehale
Copy link
Author

Thanks!

@mikehale mikehale deleted the rollbar-custom-context branch March 17, 2017 16:48
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.

2 participants