feat(profiling): add profiling to debug_files commands

[viglia]

No description provided.

[kamilogorek]

It looks like you rebased something on top of latest master branch and my recent commit is showing up here. Can you fix that please?

[kamilogorek]

As for the tracing code itself, I need to ping @Swatinem as I've never used it in Rust.

[kamilogorek]

Specifying url should be unnecessary here.

[viglia]

Without the url it's going to pull the latest published crate I think.

I'm not sure made it to a release yet, plus I'm about to merge a PR to align to a new standard format for profiles across all the SDKs so that is def. not in any release yet.

We can coordinate this with @Swatinem to make sure we make a release and at that point we can use a specific version instead of pulling from master ?

[kamilogorek]

getsentry/sentry-rust#504 is already approved, so lets merge it, release new sentry-rust and replace it with regular cargo index as it was previously.

[viglia]

I'll have to hold on for a bit for that.

I can merge that yet as the related Relay, Sentry and Snuba PRs have yet to be merged.

Once those are merged I'll be able to merge the sentry-rust, than I can use that as you suggested.

[kamilogorek]

Fixed linting issue.

[Swatinem]

I believe this example gives us a nice improvement opportunity for the global sentry API (wrapping a closure in a transaction)

I’m also not sure if some of these commands panic instead of returning an error, @kamilogorek can enlighten us here. I don’t think we currently finish a transaction when a panic occurs, and I’m also not sure that would work correctly the way that transactions can be shared by multiple hubs, or if that would even be desirable from a product point of view.

[Swatinem]

In the CI job you use override which means cargo +nightly is the same as nightly, but I think here the second command is actually using stable Rust vs nightly.
Not quite sure how exactly build-std is supposed to work, I would need to look that up.

[viglia]

When I ran my experiments (both on Intel Mac and Linux VM) that seemed to work fine.

I'd run the second command without first enabling the nightly toolchain but simply using the stable one.

[Swatinem]

the SDK is still used for other things in sentry-cli. Only the profiling (and frame pointer) feature needs to be optional in this case.

[viglia]

I think that sentry get imported in line 43 in the "normal" dependency.

This is more for the optional one at line 90 specific to Unix.

I can't use the "package-name?/feature-name" syntax to simplify this because as you said sentry as a dependency is already used regardless of profiling and I would end up adding profiling even when I should not.

Not sure if there is a better way to handle this case but I'm open for suggestions.

[kamilogorek]

I’m also not sure if some of these commands panic instead of returning an error, @kamilogorek can enlighten us here.

Yes, it can panic, and we have a hook for that in place as well

sentry-cli/src/utils/system.rs

Lines 140 to 170 in 0c4c6de

	/// Initializes the backtrace support
	pub fn init_backtrace() {
	std::panic::set_hook(Box::new(|info| {
	let backtrace = backtrace::Backtrace::new();
	let thread = std::thread::current();
	let thread = thread.name().unwrap_or("unnamed");
	let msg = match info.payload().downcast_ref::<&'static str>() {
	Some(s) => *s,
	None => match info.payload().downcast_ref::<String>() {
	Some(s) => &**s,
	None => "Box<Any>",
	},
	};
	match info.location() {
	Some(location) => {
	eprintln!(
	"thread '{}' panicked at '{}': {}:{}\n\n{:?}",
	thread,
	msg,
	location.file(),
	location.line(),
	backtrace
	);
	}
	None => eprintln!("thread '{}' panicked at '{}'{:?}", thread, msg, backtrace),
	}
	}));
	}

[kamilogorek]

I'd revert all the changes from Wraps command in a closure to make sure the transaction ends regardless of the results commit tbh, it's very noisy, and changes this PRs diff by hundreds of lines.

Cleaner solution IMO (at least for now), would be to modify debug_files/mod.rs, where we actually register and execute those commands. This way commands are self-contained, and profiling is only added as sort of "instrumentation" of those commands.

[kamilogorek]

I wonder if its worth it to move traces_sample_rate and profiles_sample_rate to constants.rs

[github-actions]

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[szokeasaurusrex]

@viglia looks like this PR has been open for some time. Are the changes still relevant, and if yes, is the PR ready for review?

[viglia]

@szokeasaurusrex, I'm closing it since it's not relevant anymore.