Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: Fix Block Viewer for Compactor and Store #3098

Merged
merged 3 commits into from
Aug 28, 2020
Merged

ui: Fix Block Viewer for Compactor and Store #3098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
996524e
ui: Fix Block viewer for Compactor and Store
onprem Aug 28, 2020
159bbec
Add an entry in CHANGELOG.md
onprem Aug 28, 2020
b5b7d3d
ui: react: Fixed tests for Blocks
onprem Aug 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ We use *breaking :warning:* word for marking changes that are not backward compa
### Fixed

* [#3095](https://github.com/thanos-io/thanos/pull/3095) Rule: update manager when all rule files are removed.
* [#3098](https://github.com/thanos-io/thanos/pull/3098) ui: Fix Block Viewer for Compactor and Store
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this, as it's fixing the release directly. But that's ok I will adjust it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


## [v0.15.0-rc.0](https://github.com/thanos-io/thanos/releases/tag/v0.15.0-rc.0) - 2020.08.26

Expand Down
19 changes: 12 additions & 7 deletions cmd/thanos/compact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,12 @@ func runCompact(
compactorView := ui.NewBucketUI(
logger,
conf.label,
path.Join(conf.webConf.externalPrefix, "/loaded"),
conf.webConf.externalPrefix,
conf.webConf.prefixHeaderName,
"/loaded",
component,
)
api := blocksAPI.NewBlocksAPI(logger, conf.label, flagsMap)
var sy *compact.Syncer
{
// Make sure all compactor meta syncs are done through Syncer.SyncMeta for readability.
Expand All @@ -229,7 +232,10 @@ func runCompact(
duplicateBlocksFilter,
}, []block.MetadataModifier{block.NewReplicaLabelRemover(logger, conf.dedupReplicaLabels)},
)
cf.UpdateOnChange(compactorView.Set)
cf.UpdateOnChange(func(blocks []metadata.Meta, err error) {
compactorView.Set(blocks, err)
api.SetLoaded(blocks, err)
})
sy, err = compact.NewSyncer(
logger,
reg,
Expand Down Expand Up @@ -395,12 +401,11 @@ func runCompact(
r := route.New()

ins := extpromhttp.NewInstrumentationMiddleware(reg)
compactorView.Register(r, ins)
compactorView.Register(r, true, ins)

global := ui.NewBucketUI(logger, conf.label, path.Join(conf.webConf.externalPrefix, "/global"), conf.webConf.prefixHeaderName)
global.Register(r, ins)
global := ui.NewBucketUI(logger, conf.label, conf.webConf.externalPrefix, conf.webConf.prefixHeaderName, "/global", component)
global.Register(r, false, ins)

api := blocksAPI.NewBlocksAPI(logger, conf.label, flagsMap)
// Configure Request Logging for HTTP calls.
opts := []logging.Option{logging.WithDecider(func() logging.Decision {
return logging.NoLogCall
Expand All @@ -413,7 +418,7 @@ func runCompact(
f := baseMetaFetcher.NewMetaFetcher(extprom.WrapRegistererWithPrefix("thanos_bucket_ui", reg), nil, nil, "component", "globalBucketUI")
f.UpdateOnChange(func(blocks []metadata.Meta, err error) {
global.Set(blocks, err)
api.Set(blocks, err)
api.SetGlobal(blocks, err)
})

srv.Handle("/", r)
Expand Down
26 changes: 22 additions & 4 deletions cmd/thanos/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package main
import (
"context"
"fmt"
"path"
"time"

"github.com/go-kit/kit/log"
Expand All @@ -17,12 +16,15 @@ import (
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
"github.com/prometheus/common/route"
blocksAPI "github.com/thanos-io/thanos/pkg/api/blocks"
"github.com/thanos-io/thanos/pkg/block"
"github.com/thanos-io/thanos/pkg/block/metadata"
"github.com/thanos-io/thanos/pkg/component"
"github.com/thanos-io/thanos/pkg/extflag"
"github.com/thanos-io/thanos/pkg/extprom"
extpromhttp "github.com/thanos-io/thanos/pkg/extprom/http"
"github.com/thanos-io/thanos/pkg/gate"
"github.com/thanos-io/thanos/pkg/logging"
"github.com/thanos-io/thanos/pkg/model"
"github.com/thanos-io/thanos/pkg/objstore/client"
"github.com/thanos-io/thanos/pkg/prober"
Expand Down Expand Up @@ -149,6 +151,7 @@ func registerStore(m map[string]setupFunc, app *kingpin.Application) {
*webPrefixHeaderName,
*postingOffsetsInMemSampling,
cachingBucketConfig,
getFlagsMap(cmd.Model().Flags),
)
}
}
Expand Down Expand Up @@ -180,6 +183,7 @@ func runStore(
externalPrefix, prefixHeader string,
postingOffsetsInMemSampling int,
cachingBucketConfig *extflag.PathOrContent,
flagsMap map[string]string,
) error {
grpcProbe := prober.NewGRPC()
httpProbe := prober.NewHTTP()
Expand Down Expand Up @@ -363,9 +367,23 @@ func runStore(
// Add bucket UI for loaded blocks.
{
r := route.New()
compactorView := ui.NewBucketUI(logger, "", path.Join(externalPrefix, "/loaded"), prefixHeader)
compactorView.Register(r, extpromhttp.NewInstrumentationMiddleware(reg))
metaFetcher.UpdateOnChange(compactorView.Set)
ins := extpromhttp.NewInstrumentationMiddleware(reg)

compactorView := ui.NewBucketUI(logger, "", externalPrefix, prefixHeader, "/loaded", component)
compactorView.Register(r, true, ins)

// Configure Request Logging for HTTP calls.
opts := []logging.Option{logging.WithDecider(func() logging.Decision {
return logging.NoLogCall
})}
logMiddleware := logging.NewHTTPServerMiddleware(logger, opts...)
api := blocksAPI.NewBlocksAPI(logger, "", flagsMap)
api.Register(r.WithPrefix("/api/v1"), tracer, logger, ins, logMiddleware)

metaFetcher.UpdateOnChange(func(blocks []metadata.Meta, err error) {
compactorView.Set(blocks, err)
api.SetLoaded(blocks, err)
})
srv.Handle("/", r)
}

Expand Down
6 changes: 3 additions & 3 deletions cmd/thanos/tools_bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,8 @@ func registerBucketWeb(m map[string]setupFunc, root *kingpin.CmdClause, name str
router := route.New()
ins := extpromhttp.NewInstrumentationMiddleware(reg)

bucketUI := ui.NewBucketUI(logger, *label, *webExternalPrefix, *webPrefixHeaderName)
bucketUI.Register(router, ins)
bucketUI := ui.NewBucketUI(logger, *label, *webExternalPrefix, *webPrefixHeaderName, "", component.Bucket)
bucketUI.Register(router, true, ins)

flagsMap := getFlagsMap(cmd.Model().Flags)

Expand Down Expand Up @@ -392,7 +392,7 @@ func registerBucketWeb(m map[string]setupFunc, root *kingpin.CmdClause, name str
}
fetcher.UpdateOnChange(func(blocks []metadata.Meta, err error) {
bucketUI.Set(blocks, err)
api.Set(blocks, err)
api.SetGlobal(blocks, err)
})

ctx, cancel := context.WithCancel(context.Background())
Expand Down
42 changes: 30 additions & 12 deletions pkg/api/blocks/v1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ import (

// BlocksAPI is a very simple API used by Thanos Block Viewer.
type BlocksAPI struct {
baseAPI *api.BaseAPI
logger log.Logger
blocksInfo *BlocksInfo
baseAPI *api.BaseAPI
logger log.Logger
globalBlocksInfo *BlocksInfo
loadedBlocksInfo *BlocksInfo
}

type BlocksInfo struct {
Expand All @@ -35,7 +36,11 @@ func NewBlocksAPI(logger log.Logger, label string, flagsMap map[string]string) *
return &BlocksAPI{
baseAPI: api.NewBaseAPI(logger, flagsMap),
logger: logger,
blocksInfo: &BlocksInfo{
globalBlocksInfo: &BlocksInfo{
Blocks: []metadata.Meta{},
Label: label,
},
loadedBlocksInfo: &BlocksInfo{
Blocks: []metadata.Meta{},
Label: label,
},
Expand All @@ -51,19 +56,32 @@ func (bapi *BlocksAPI) Register(r *route.Router, tracer opentracing.Tracer, logg
}

func (bapi *BlocksAPI) blocks(r *http.Request) (interface{}, []error, *api.ApiError) {
return bapi.blocksInfo, nil, nil
viewParam := r.URL.Query().Get("view")
if viewParam == "loaded" {
return bapi.loadedBlocksInfo, nil, nil
}
return bapi.globalBlocksInfo, nil, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels weird from perspective on components that does not have global 🤔

Also I think it's on purpose that in code we talk about global view and in React / UI we have blocks?

Copy link
Member Author

@onprem onprem Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is weird. But in our last 1:1, me, @squat, and @GiedriusS discussed and decided to go with this. As we have the same API but different needs in different components (Bucket viewer: a global view of blocks, Compactor: both global and loaded and Store: loaded only) we came up with two solutions:

  • Modify the API logic so that we can register a single type of API multiple times in a single component with different prefixes. This means that in the Compactor, we can create two instances of Blocks API on two different prefixes (let's say /api/v1/loaded and /api/v1/global) much like how we handled the old UI.
  • Extend the blocks API to have two types of data (for Global and Loaded) always and add a query parameter in the existing /api/v1/blocks endpoint.

The first solution is more generic but it requires us to modify logic for all of the API packages. Also, it would result in some weird endpoints like /api/v1/loaded/blocks and /api/v1/global/blocks (ideally the level of specificity should increase, /api/v1/blocks/loaded) because we can only add a prefix to a router. The second solution was simpler to implement and only affected the Blocks API. So even though it felt a little weird, we decided to go with it.

}

// Set updates the blocks' metadata in the API.
func (bapi *BlocksAPI) Set(blocks []metadata.Meta, err error) {
func (b *BlocksInfo) set(blocks []metadata.Meta, err error) {
if err != nil {
// Last view is maintained.
bapi.blocksInfo.RefreshedAt = time.Now()
bapi.blocksInfo.Err = err
b.RefreshedAt = time.Now()
b.Err = err
return
}

bapi.blocksInfo.RefreshedAt = time.Now()
bapi.blocksInfo.Blocks = blocks
bapi.blocksInfo.Err = err
b.RefreshedAt = time.Now()
b.Blocks = blocks
b.Err = err
}

// SetGlobal updates the global blocks' metadata in the API.
func (bapi *BlocksAPI) SetGlobal(blocks []metadata.Meta, err error) {
bapi.globalBlocksInfo.set(blocks, err)
}

// SetLoaded updates the local blocks' metadata in the API.
func (bapi *BlocksAPI) SetLoaded(blocks []metadata.Meta, err error) {
bapi.loadedBlocksInfo.set(blocks, err)
}
Loading