feat: add tracing to proto validation#2950
Conversation
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (74.78%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2950 +/- ##
==========================================
+ Coverage 73.82% 74.78% +0.96%
==========================================
Files 494 495 +1
Lines 60623 60638 +15
==========================================
+ Hits 44750 45343 +593
+ Misses 12698 12132 -566
+ Partials 3175 3163 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1cc318f to
cb441f1
Compare
52f9626 to
b078558
Compare
59b3c1b to
9a345fb
Compare
| @@ -1,2 +1,2 @@ | |||
| // Package handwrittenvalidation defines middleware that runs custom-made validations on incoming requests. | |||
| // Package handwrittenvalidation defines middleware that runs validations on incoming requests. | |||
| package handwrittenvalidation | |||
There was a problem hiding this comment.
maybe i should rename the package...
There was a problem hiding this comment.
We also might be able to get rid of this package? I haven't actually looked into it
| @@ -1,2 +1,2 @@ | |||
| // Package handwrittenvalidation defines middleware that runs custom-made validations on incoming requests. | |||
| // Package handwrittenvalidation defines middleware that runs validations on incoming requests. | |||
| package handwrittenvalidation | |||
There was a problem hiding this comment.
We also might be able to get rid of this package? I haven't actually looked into it
There was a problem hiding this comment.
I don't think folding together the validation logic makes sense to me. If the goal is just "add spans around an interceptor," I'd rather write a function that takes an interceptor and a name and returns a new interceptor that adds spans with that name around the provided interceptor, and then we can throw it around the protovalidate interceptor. I think there's a good chance that we can get rid of this handwritten logic altogether using CEL rules that reference multiple fields and I'd rather not tie them together.
There was a problem hiding this comment.
I don't think folding together the validation logic makes sense to me.
why not? the responsibilties are the same: validate the incoming request.
the split can cause bugs. Look at the existing codebase today, in particular the Watch service: https://github.com/authzed/spicedb/blob/main/internal/services/v1/watch.go#L39. Notice something odd? if there were a handwritten validation defined, this code is buggy, it's not running it.
I'd rather write a function that takes an interceptor and a name and returns a new interceptor that adds spans with that name around the provided interceptor,
agree, i want to do this and put a span around EVERY interceptor because there are so many and i don't know where the time is being spent. but: separate PR
think there's a good chance that we can get rid of this handwritten logic altogether using CEL rules that reference multiple fields and I'd rather not tie them together.
agree, but: separate PR
9a345fb to
5c8d195
Compare
5c8d195 to
1fad59e
Compare
1fad59e to
3f35624
Compare
|
|
||
| // WrapUnaryServerInterceptorWithSpans returns a new interceptor that wraps the given interceptor | ||
| // with a span, measuring the duration of the interceptor's pre-handler logic. | ||
| func WrapUnaryServerInterceptorWithSpans( |
There was a problem hiding this comment.
@tstirrat15 is the idea to wrap more interceptors with this?
There was a problem hiding this comment.
That's the idea - it's here if we want it.
3f35624 to
b7a0872
Compare
tstirrat15
left a comment
There was a problem hiding this comment.
Maria approved my edits; I'm approving the PR.
Description
add tracing to proto validation
Testing
References