errorsbp package #239
errorsbp package #239
Conversation
We are about to add more stuff into that package, so the specific name of batcherror no longer makes sense.
|
This replaces #225 |
thriftbp/thrifttest/server.go
Outdated
| @@ -157,6 +157,7 @@ func (s *Server) Close() error { | |||
| func NewBaseplateServer( | |||
| store *secrets.Store, | |||
| processor thrift.TProcessor, | |||
| suppressor errorsbp.Suppressor, | |||
There was a problem hiding this comment.
I'm not super happy with this function signature change, but this function also makes less sense to convert to a struct arg.
There was a problem hiding this comment.
I think this probably makes sense to put in ServerConfig?
There was a problem hiding this comment.
That's the tricky part. ServerConfig is used by NewServer, which is the more customized version of NewBaseplateServer. NewBaseplateServer actually constructs ServerConfig and calls NewServer underneath.
In NewServer, this is not needed as NewServer does not create any middlewares by itself, it just applies all the middlewares passed in.
This is also the reason I said this function makes less sense to have a struct arg, because that new struct will be very confusing alongside ServerConfig.
There was a problem hiding this comment.
OK I changed ServerConfig to be used by both NewServer and NewBaseplateServer. This makes the function signatures much better.
There was a problem hiding this comment.
Ya, I agree that neither option is ideal. Looking forward though, I could see a world where we deprecate and remove NewServer, and NewBaseplateServer is the real focus anyways.
| @@ -29,6 +32,6 @@ | |||
| // return batch.Compile() | |||
| // } | |||
| // | |||
| // This package is not thread-safe. | |||
| // errorsbp.Batch is not thread-safe. | |||
| // The same batch should not be operated on different goroutines concurrently. | |||
There was a problem hiding this comment.
Should we add Suppressor to this too?
There was a problem hiding this comment.
Yes I planned to but forgot. Added.
thriftbp/thrifttest/server.go
Outdated
| @@ -157,6 +157,7 @@ func (s *Server) Close() error { | |||
| func NewBaseplateServer( | |||
| store *secrets.Store, | |||
| processor thrift.TProcessor, | |||
| suppressor errorsbp.Suppressor, | |||
There was a problem hiding this comment.
I think this probably makes sense to put in ServerConfig?
|
💇 |
| // regarding how it is used. | ||
| ErrorSpanSuppressor errorsbp.Suppressor | ||
|
|
||
| // Optional, used only by NewServer. |
There was a problem hiding this comment.
I don't think we really want to say used only by NewServer? I think that's confusing since it's still used by NewBaseplateServer because that calls NewServer.
There was a problem hiding this comment.
It's "used only by NewServer" in the sense of when you pass ServerConfig into NewBaseplateServer, that field will be completely ignored by NewBaseplateServer function (and overwritten by the actual address extracted from bp.Config()). So that field is, indeed, used only by NewServer.
There was a problem hiding this comment.
I agree that it is technically correct but I think it's misleading and can cause confusion.
There was a problem hiding this comment.
NewBaseplateServerusesNewServerso the field is still used byNewBaseplateServer.- I don't think it's important to people trying to read the documentation that
NewBaseplateServercallsNewServerand if that ever changes, we'll likely have to use it directly inNewBaseplateServer. From the perspective of someone trying to integrate with this, it is used byNewBaseplateServerbecause that's all they interact with.
There was a problem hiding this comment.
Ok, I think this works well 🐟
| // | ||
| // The suppressor arg will be used as | ||
| // DefaultProcessorMiddlewaresArgs.ErrorSpanSuppressor, | ||
| // please refer to its documentation for more details. |
There was a problem hiding this comment.
It might be good to add that the Addr and Timeout will be pulled from bp.Config()
Also update both server and client middlewares in thriftbp to support it, and also function signature changes to NewServer, NewBaseplateServer, etc. to support the new arguments.
With
batcherror.BatchErrorrenamed intoerrorsbp.Batch, anderrorsbp.Suppressordefined.