WIP: Fix1858: Ability to search all tenants in multi-tenant set up #2854
Conversation
mghildiy
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
kvrhdn,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
|
Its WIP. Test part is pending |
| logger log.Logger | ||
| } | ||
|
|
||
| func (p parseQuery) RoundTrip(r *http.Request) (*http.Response, error) { |
There was a problem hiding this comment.
we will need this same functionality on all paths: tags, metrics, trace by id and search. i think we should try to implement this as a generic piece of middleware.
the middleware would define a "combiner" interface that it used to recombine the incoming http requests. wdyt?
There was a problem hiding this comment.
Ok. So every path then would provide implementation for this new combiner interface as per path's requirements.
There was a problem hiding this comment.
yes, but tbh i don't think this is possible atm on the metrics endpoints. maybe just return an error there for now since it's experimental?
There was a problem hiding this comment.
I check that search and trace by id have diff ways of processing requests and combining the responses. So another approach may be to pass this behaviour as a function to the middleware.
There was a problem hiding this comment.
type Combiner interface {
Consume(input any) (count int)
Result(statusCode int, statusMsg string) (*http.Response, error)
}
This is the Combiner interface I plan to have.
Consume is for merging responses of individual requests.
Result is for putting together final response based on merge result(and final statuscode, status message after all requests are processed).
There was a problem hiding this comment.
Consume()should take an*http.Response. the middleware should just give it all responses it receives and rely on the interface's implementation to work out the details- Does result need the status code/message? These values will be in each consumed http.response?
I'm thinking like:
type Combiner interface {
Consume(*http.Response)
Result() (*http.Response, error)
}
as discussed out of band, let's:
- make this configurable (off by defafult)
- only worry about the trace by id path right now
if we establish a good generic set of code we can add the others later.
There was a problem hiding this comment.
As consuming a response may result into an error too(like when reading response body or during unmarshalling the body), so Consume must return error which we then use to build final response, and then maybe we call it Process.
Process(*http.Response) error
There was a problem hiding this comment.
I'm fine with either Process or Result. I think they are clear enough
There was a problem hiding this comment.
Added a cross tenant middleware which takes combiner as input, and added one such combiner for traces
There was a problem hiding this comment.
Please review it. I plan to modify test accordingly.
mghildiy
force-pushed
the
fix1858
branch
2 times, most recently
from
5e6e857
to
c19a069
Compare
|
Thank you, @mghildiy for putting in time into this feature. This feature requires significant efforts and we are thankful for your time and efforts you have put into this. As mentioned here, We have decided to take this feature from our enterprise version(GET) and move it into OSS Tempo. We believe that it's better if someone from the team takes our enterprise code and moves it into OSS tempo. It will avoid the duplicate efforts. I am working on the PR to move it from Enterprise to OSS and will have a PR out soon. Thank you for the contribution ❤️ |
|
This PR has been automatically marked as stale because it has not had any activity in the past 60 days. |
Adds ability to search all tenants in multi-tenant setup:
Which issue(s) this PR fixes:
Fixes #1858
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]