Skip to content

Upgrade CLI to v2 and added arbitrary ranges#16

Merged
metalmatze merged 12 commits into
go-pluto:masterfrom
mfranzil-experiments:master
Jun 20, 2023
Merged

Upgrade CLI to v2 and added arbitrary ranges#16
metalmatze merged 12 commits into
go-pluto:masterfrom
mfranzil-experiments:master

Conversation

@mfranzil
Copy link
Copy Markdown
Contributor

@mfranzil mfranzil commented Jun 7, 2023

See issue #15.

This PR improves on the tool by:

  • updating to Go 1.18,
  • updating the urfave/cli package to v2 (with relative changes)
  • adding the new Go files to .gitignore,

and most importantly, introduces the possibility of providing a start interval and a duration, rather than a duration which is calculated backwards from the current time. Might need some extra work (as I used a questionable approach for the steps function in prometheus.go), and probably the vendoring could also be removed.

Copy link
Copy Markdown
Contributor

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

Thank you for posting it!
There are a couple of things we should address before merging the PR. Overall it looks really good!

Comment thread README.md Outdated
Comment thread go.mod Outdated
Comment thread go.mod Outdated
Comment thread prometheus.go
@@ -88,13 +88,7 @@ func Query(host string, start time.Time, end time.Time, query string) ([]Result,
}

func steps(dur time.Duration) int {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, this might result in very high amount data, for exporting data with 1w (or even longer) time ranges.

@mfranzil
Copy link
Copy Markdown
Contributor Author

I have fixed the first three issues (and in the meantime, also removed a legacy feature of the old cli v1 API, we can just obtain the parameters without creating structs)

However, I'm kind of unsure how to fix the last one. Should we add options for granularity? We should also probably be more transparent to the user when diminishing it. When I first used the tool, I was confused why having long time ranges had such a low amount of data points.

@metalmatze
Copy link
Copy Markdown
Contributor

Yeah, it all depends on the expectation. What we usually do in UI is to have the duration be a pixel on the canvas. For example, given you have 1000 pixel width, for one week we'd do (7*24*60*60)/1000 ~ 600s = 10m. Now, this doesn't really work here. We could get the raw output, or have a parameter that specifies how many datapoints someone expects.

Either way, it seems we can do that one on a follow-up PR.

Thank you so much for these changes!

@metalmatze metalmatze merged commit a4f4604 into go-pluto:master Jun 20, 2023
@mfranzil
Copy link
Copy Markdown
Contributor Author

Yeah, it all depends on the expectation. What we usually do in UI is to have the duration be a pixel on the canvas. For example, given you have 1000 pixel width, for one week we'd do (7*24*60*60)/1000 ~ 600s = 10m. Now, this doesn't really work here. We could get the raw output, or have a parameter that specifies how many datapoints someone expects.

Either way, it seems we can do that one on a follow-up PR.

Thank you so much for these changes!

See #18 as a follow-up. You're welcome!
(Also sorry if I respond so late, I tend to procrastinate stuff in my inbox too much ahah)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants