fix: Bump failure counter on job error

[tomasfarias]

Also bump webhook_jobs_failed on job error. Maybe we should have a separate counter for this? PgJobError usually indicates something on our end, so it could be worth it to have a distinction.

EDIT: I've done that, there is now a webhook_jobs_database_error counter.

[bretthoerner]

It's minor, but I think we should have a different enum entry like DatabaseError too. Just helps for grep in the future.

[bretthoerner]

Doesn't have to be for this PR btw.

edit: It might help for distinguishing PgJobError or DatabaseError in tests, i.e. here: https://github.com/PostHog/rusty-hook/pull/36/files#diff-3c41c606a4d1c83ee33ba25992dd4d7a4288bd20270e68f210e086a1101462acR335

[bretthoerner]

Hmm, I was thinking in my head things like complete() returned a sqlx::Error but I guess those are wrapped internally:

rusty-hook/hook-common/src/pgqueue.rs

Lines 30 to 37 in ead1bf2

	pub enum PgJobError<T> {
	#[error("retry is an invalid state for this PgJob: {error}")]
	RetryInvalidError { job: T, error: String },
	#[error("{command} query failed with: {error}")]
	QueryError { command: String, error: sqlx::Error },
	#[error("transaction {command} failed with: {error}")]
	TransactionError { command: String, error: sqlx::Error },
	}

It's a little weird that RetryInvalidError will get treated as a DB error here. Although I guess that should never happen, since we're just calling complete/fail, it feels like the types could help us better here somehow.

[tomasfarias]

Yeah, I agree we should differentiate between RetryInvalidError and database errors. RetryInvalidError is the only special error as it returns the job itself to be failed (otherwise we would be consuming it), so I'd split it off this enum and rename the enum to DatabaseError or something similar.

[tomasfarias]

Happy to update this PR, will need a bit of time as I have some support tickets to work through.

[bretthoerner]

Happy to update this PR, will need a bit of time as I have some support tickets to work through.

No big deal, it was mostly just a thought. Feel free to merge this if you want too, either way.

[bretthoerner]

I totally forgot about this one, @tomasfarias did you want to merge this as-is?

[tomasfarias]

@bretthoerner

Not sure if tests will pass, but given our conversation I've refactored PgQueueError to separate RetryInvalidError into its own struct (at the same time making type signatures simpler for methods that aren't retry). Now it should be easier to differentiate between DatabaseErrors and RetryInvalidError.

[tomasfarias]

Looks like test failures are (for now) due to me making new_from_pool non-fallible (as it could not fail). Can clean those up later...

[tomasfarias]

Waiting for #60 to be merged as there will be conflicts to fix, and makes more sense to tackle them here.

[tomasfarias]

Rebased on main after #60 was merged. This should be fine to merge too assuming tests are green!