-
Notifications
You must be signed in to change notification settings - Fork 105
Conversation
Currently generates deterministic metrics for each metrictank partition, but does not yet validate them.
cmd/mt-fakemetrics/cmd/parrot.go
Outdated
|
||
func init() { | ||
rootCmd.AddCommand(parrotCmd) | ||
parrotCmd.Flags().IntVar(&parrotOrgId, "parrot-org-id", 1, "org id to publish parrot metrics to") |
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.
Does this make more sense as a default than not setting an org ID?
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.
Related: I've made the assumption that we're only going to send parrot metrics to a single org.
This disallows partitioning by org.
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.
1 is a fine default. single org is fine too I think.
cmd/mt-fakemetrics/cmd/parrot.go
Outdated
func init() { | ||
rootCmd.AddCommand(parrotCmd) | ||
parrotCmd.Flags().IntVar(&parrotOrgId, "parrot-org-id", 1, "org id to publish parrot metrics to") | ||
parrotCmd.Flags().Int32Var(&parrotPartitionCount, "parrot-partition-count", 128, "number of partitions to publish parrot metrics to") |
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.
So the partition count is a little tricky.
If we're using mt-gateway (which I think is the primary intended use case), we need to pass it in as a flag.
If we're using kafka, we should really probably have a way to discover it via kafka. Does it make sense to disable kafka as an output?
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 now we can keep it simple and don't do discovery. in fact we may as well just assume that we will always have 1 grafananet/tsdb-gw/mt-gateway (these areall the same) output, and not kafka.
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.
Yeah, and that becomes a lot easier once I move it to a top level command and don't have the existing root command kafka flags cluttering it up
cmd/mt-fakemetrics/cmd/parrot.go
Outdated
} | ||
|
||
var parrotCmd = &cobra.Command{ | ||
Use: "parrot", |
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.
initial card said parrot-tests
, do we want to change to that?
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.
no. there is no need for the "-tests" suffix AFAICT
cmd/mt-fakemetrics/cmd/parrot.go
Outdated
if orgs > 1 { | ||
log.Fatal("parrot only works in single org mode") | ||
} | ||
initStats(false, "parrot") |
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.
planning on reporting different stats on parrot query results, not sure if the default stats make sense or not?
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.
they are useful though not critical
mainly I think we should report at each point in time the number of metrics that are incorrect.
cmd/mt-fakemetrics/cmd/parrot.go
Outdated
@@ -0,0 +1,102 @@ | |||
package cmd |
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.
this tool is beyond just sending fake metrics.
as this will send fake data into a MT cluster and then query it back, it's a "real" MT testing tool, so i would put it in /cmd/mt-parrot. you then also don't have to do the cobra stuff.
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.
to be clear, we don't want a tool that just sends the test data.
we want a tool that can execute the entire test.
it can use the fakemetrics library to do its work.
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.
understood, was planning on adding that in next, just sanity checking this portion before building on it
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.
also i realize this is a departure from the suggestion i have given you before (which was a new mode for fakemetrics tool + a separate query/test tool), but it seems to me doing both in one tool is simpler.
cmd/mt-fakemetrics/cmd/parrot.go
Outdated
|
||
var ( | ||
parrotOrgId int | ||
parrotPartitionCount int32 |
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.
these vars don't need the 'parrot' prefix AFAICT
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.
They don't need it, but I felt like the cmd/fakemetrics
namespace was getting a little crowded. No longer a concern if we're moving to cmd/mt-parrot
let's also add the testing to the tool, and submitting the fail/success stats |
Gonna close this for now, expecting a decent amount of churn during troubleshooting |
Currently generates deterministic metrics for each metrictank partition, but does not yet validate them.
Checks are probably going to fail initially due to doc checks, gonna hold off on generating them initially.