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

Feature rollbar #37

Merged
merged 4 commits into from
Jan 22, 2021
Merged

Feature rollbar #37

merged 4 commits into from
Jan 22, 2021

Conversation

mijoharas
Copy link
Contributor

two questions. Firstly, I made rollbar a feature so you build with cargo build --features rollbar.

  1. should I change the makefile to build with this feature by default?
  2. should I make this feature a default feature?

Also, I want to draw your attention to this which gives me pause: RoxasShadow/rollbar-rs#16 I think it's probably fine though? thoughts?

std::env::var("ROLLBAR_ACCESS_TOKEN").expect("ROLLBAR_ACCESS_TOKEN env is not set");
static ref ROLLBAR_CLIENT: rollbar::Client =
rollbar::Client::new((*ROLLBAR_ACCESS_TOKEN).clone(), "development".to_owned());
static ref LAST_ROLLBAR_THREAD_HANDLE: std::sync::Mutex<Option<std::thread::JoinHandle<Option<ResponseStatus>>>> =
Copy link
Contributor Author

@mijoharas mijoharas Jan 22, 2021

Choose a reason for hiding this comment

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

to explain this more explicitly. The rollbar library will just start a thread to send the message (so probably not great architecture if there are lots of things rollbar-ing, but hopefully that's not the case, so no worries).

When our application ends, we kill any extant threads. This keeps us alive until the most recently sent rollbar message thread completes (NOTE: this does not ensure all threads will have processed their messages, but I didn't really want to have a thread vec that we need to constantly maintain and check whether things are finished to ensure we're perfect, this seemed like a good middle ground between correct and simple).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, makes sense 👍

Copy link
Contributor

@joshuafleck joshuafleck left a comment

Choose a reason for hiding this comment

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

🚀

[features]

# Feature names must not conflict with other package names in the manifest. This is because they are opted into via features = [...], which only has a single namespace.
with_rollbar = ["rollbar", "backtrace"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe stick something in the readme about this?

@mijoharas mijoharas merged commit 350dac8 into master Jan 22, 2021
joshuafleck pushed a commit that referenced this pull request Aug 15, 2022
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