-
Notifications
You must be signed in to change notification settings - Fork 605
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.
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
feat: profilecli query-blocks series #3610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracting some common logic from query.go
2e85454
to
6f7e9c5
Compare
@@ -159,6 +159,23 @@ func (b *BlockQuerier) BlockMetas(ctx context.Context) (metas []*block.Meta, _ e | |||
return metas[0 : pos+1], nil | |||
} | |||
|
|||
func (b *BlockQuerier) BlockMeta(ctx context.Context, name string) (meta *block.Meta, _ error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to introduce a direct way to read a single meta, otherwise, using BlockMetas
would load every block in the bucket (potentially thousands of them)
6f7e9c5
to
7bc0d15
Compare
3b727d3
to
40b3777
Compare
40b3777
to
ba8022f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
Could you expand the PR description with examples of successful calls? One with local and one with remote blocks, to make it more clear on how this can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks for implementing this @alsoba13!
cmd/profilecli/output.go
Outdated
for k := range m { | ||
delete(m, k) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for k := range m { | |
delete(m, k) | |
} | |
clear(m) |
queryBlocksCmd := app.Command("query-blocks", "Query on local/remote blocks") | ||
queryBlocksSeriesCmd := queryBlocksCmd.Command("series", "Request series labels on local/remote blocks") | ||
queryBlocksSeriesParams := addQueryBlocksSeriesParams(queryBlocksSeriesCmd) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about moving this to admin blocks
sub-commands?
profilecli admin blocks --help
usage: profilecli admin blocks [<flags>] <command> [<args> ...]
Operate on Grafana Pyroscope's blocks.
Flags:
-h, --help Show context-sensitive help (also try --help-long and --help-man).
--version Show application version.
-v, --verbose Enable verbose logging.
--path="./data/local" Path to blocks directory
Subcommands:
admin blocks list [<flags>]
List blocks.
admin blocks compact [<flags>] <from> <dest>
Compact blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related to this PR, but if you think it makes sense we can improve the storage config handling (in all subcommands)
I think that we should use the standard client config (that supports all backends). You can register the flags as follows:
blocksCmd := adminCmd.Command("blocks", "Operate on Grafana Pyroscope's blocks.")
blocksCmd.Flag("path", "Path to blocks directory").Default(defaultLocalBlockPath).StringVar(&cfg.blocks.path)
var f flag.FlagSet
var c client.StorageBackendConfig
c.RegisterFlags(&f, logger)
f.VisitAll(func(flag *flag.Flag) {
blocksCmd.Flag(flag.Name, flag.Usage).Hidden().Default(flag.DefValue).SetValue(flag.Value)
})
This will probably require handling of the default path and fs backend. This way we could create our Bucket
object for all blocks
subcommands automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the flags of the storage, I'll take a look and maybe implement separately.
About making query a subcommand of admin blocks, I discarded that idea at first because I thought that profilecli admin blocks
needed a pyroscope server to work. Now I see it works directly on blocks, and makes sense to merge commands here. I may implement this as well in a following PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple minor things.
ctx := context.Background() | ||
blockQuerier := NewBlockQuerier(ctx, bucket) | ||
metas, err := blockQuerier.BlockMetas(ctx) | ||
require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add something like this to ensure there's actually data to test.
require.NoError(t, err) | |
require.NoError(t, err) | |
require.NotEmpty(t, metas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good one as well, fixed
7035d60
to
beff499
Compare
beff499
to
9f18c3a
Compare
In this PR we introduce a new profilecli command:
query-blocks
. With this command, you can execute queries directly to single or multiple blocks hosted in your localhost or a remote bucket.Partially solves #3559,
profile query-blocks merge
will come in a later PR.Capabilities
This feature gives similar capabilities as
profilecli query series
but for a specified local/remote blocks:--label-names
or specify the query with--query
.--local-path
) or remotely (--bucket-name
,--tenant-id
and--object-store-type
- only gcs supported right now).--block-ids
flag.to
andfrom
) are not needed: it will query the whole blocks instead.doc
Usage example:
Querying series on a local block, and filter on service_name
Querying series on two remote blocks, just checking at
__profile_type__
label: