Feature/remote writes status page (#7971) #8218
Conversation
strideynet
force-pushed
the
feature/remote-writes-status-page
branch
2 times, most recently
from
2f64326
to
a5a5398
Compare
| shards *shards | ||
| numShards int | ||
| shards *shards | ||
| numShards int |
There was a problem hiding this comment.
This all needs locking
There was a problem hiding this comment.
Yeah, I was just roughly drafting out on the structs to get a feel. Need to go back and make sure appropriate mutexes are added.
web/ui/react-app/src/pages/remoteWrites/RemoteWritesContent.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Noah Stride <noah@noahstride.co.uk>
strideynet
force-pushed
the
feature/remote-writes-status-page
branch
from
d466a19
to
3277706
Compare
| ) | ||
|
|
||
| if samplesOutRate <= 0 { | ||
| var c ShardingCalculations |
There was a problem hiding this comment.
Can't tell if this version using the struct is too much uglier, and if it would make more sense to just copy the values in at the end?
There was a problem hiding this comment.
I slightly prefer copying the stats at the end, but not a big deal.
Signed-off-by: Noah Stride <noah@noahstride.co.uk>
There was a problem hiding this comment.
Awesome work so far!
A few things I'm a bit unsure on is how we should best extract the pending samples, and last send, last send duration and last error status for each shard. My current thought is introducing a map to the shards of shard id to some kind of ShardStatus struct that contains each of these values and then to update this "status" struct from each shards goroutine.
That seems reasonable to me, an alternative would be a new shard struct that would then have a run method on it in place of runShard. The nice part of that would be each shard would then be independent of each other and not need to know about a shared state map. shards would then have to have a []*shard, but that makes sense from a dependency perspective anyway.
| ) | ||
|
|
||
| if samplesOutRate <= 0 { | ||
| var c ShardingCalculations |
There was a problem hiding this comment.
I slightly prefer copying the stats at the end, but not a big deal.
| Queues: make([]*RemoteWriteQueue, 0, len(queues)), | ||
| } | ||
|
|
||
| var wrn []error |
web/api/v1/api_remote_write.go
Outdated
| } | ||
|
|
||
| // RemoteWriteShardingCalculations provides insight into the values that are used to calculate the required shard num | ||
| type RemoteWriteShardingCalculations struct { |
There was a problem hiding this comment.
Rather than creating a new struct, could this just use remote.ShardingCalculations?
There was a problem hiding this comment.
Yeah, I wasn't too sure if we tried to keep all API objects defined in api/v1, I did consider just adding the json tags to the remote.ShardingCalculations
There was a problem hiding this comment.
We've a mix currently. We've had problems in cases where the actual struct didn't match what we wanted on the wire, but if that's not the case this should be okay to reuse. This API will be marked experimental one way or the other.
There was a problem hiding this comment.
Perfect, thanks for clearing that up @brian-brazil
| @@ -203,6 +203,19 @@ func (rws *WriteStorage) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Queues returns all of the queues that have been configured | |||
| func (rws *WriteStorage) Queues() []*QueueManager { | |||
There was a problem hiding this comment.
I wonder if rather than returning the actual queues if we should just return stats instead with a Stats() method? Not a strong feeling either way, but I am not sure if we need to get the full queues.
There was a problem hiding this comment.
Not too sure on how I feel on this one yet. Probably will build out the rest and then see if my opinion changes on this. It definitely feels odd to return the entire queues as you've said.
I think that alternative is definitely what I would prefer potentially, I was just cautious of making significant changes to the working code. That being said, I think I will go with your suggestion, since it seems cleaner imho. |
Signed-off-by: Noah Stride <noah@noahstride.co.uk>
Signed-off-by: Noah Stride <noah@noahstride.co.uk>
Signed-off-by: Noah Stride <noah@noahstride.co.uk>
Signed-off-by: Noah Stride <noah@noahstride.co.uk>
|
hello @strideynet , are you still up for this? :) |
|
Hey @roidelapluie, had a lot of stuff going on with switching jobs but am a bit more free now. I'm happy to come back to this weekend and work out where I was, and pick it up again, unless someone else is offering :) |
|
If you are in, this is still an awesome feature :) |
|
Too busy with $PRIMARY_EMPLOYER to finish this ticket, apologies. |
Purely a rough draft at the mo. Well aware tests are currently missing.
A few things I'm a bit unsure on is how we should best extract the pending samples, and last send, last send duration and last error status for each shard. My current thought is introducing a map to the shards of shard id to some kind of ShardStatus struct that contains each of these values and then to update this "status" struct from each shards goroutine.