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

Jump to bottom

✅ Upgrade metricsql #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

✅ Upgrade metricsql #3

wants to merge 1 commit into from

Conversation

vegerot
Copy link
Contributor

@vegerot vegerot commented Aug 26, 2022 •
edited

Note to reviewer

Changes

✅ Add tests

  • Added tests from README
  • Refactor a bit of the code to make it more easily testable

⬆️ Upgrade metricsql

  • No longer need to maintain fork
  • Correctness verified by tests

@vegerot
Copy link
Contributor Author

vegerot commented Aug 26, 2022

If the review would prefer I could make two separate PRs, one for each commit. And then merge the second PR after the first is merged

@vegerot vegerot changed the title Add tests and upgrade metricsql ✅ Add tests and upgrade metricsql Aug 26, 2022
@jiacai2050
Copy link
Owner

Thanks for your interests.

Three questions for this PR:

  • Why no squash? IMO squash can make the commits history clean.
  • Why replace forked metricsql? I forked it to support UTF-8 chars, does upstream support this out of box?
  • I prefer to not use emoji in commit message.

@vegerot
Copy link
Contributor Author

vegerot commented Aug 27, 2022 •
edited

  • Why no squash
  • These commits have nothing to do with each other, so it will make the history more searchable. Don't worry about it, I'll just make separate PRs.

Why replace forked metricsql

  • I didn't realize it didn't support non-UTF-8 chars. I just tested what was in the README. Would you mind giving me an example so I could write a test for it? (or even better, you could contribute that test)

not use emoji in commit message

done.

@vegerot vegerot marked this pull request as draft August 27, 2022 03:43
@vegerot
Copy link
Contributor Author

vegerot commented Aug 27, 2022

Marking as draft until #4 is merged

@vegerot
[upgrade deps] Upgrade metricsql
2b67bdd 
No longer need to maintain fork
Correctness verified by tests
@vegerot vegerot changed the title ✅ Add tests and upgrade metricsql ✅ Upgrade metricsql Aug 30, 2022
@vegerot vegerot marked this pull request as ready for review August 30, 2022 01:02
@vegerot
Copy link
Contributor Author

vegerot commented Aug 30, 2022

Circling back to this PR, would you please give me an example of a PromQL expression that metricsql would not parse?

jiacai2050
jiacai2050 reviewed Aug 30, 2022
View reviewed changes
util.go
@@ -46,11 +46,12 @@ func prettier(expr metricsql.Expr, ident int) []byte {
var b bytes.Buffer

wrapParensWhenNecesary(e.Expr, &b, ident)
if len(e.Window) > 0 || len(e.Step) > 0 {
b.WriteString(fmt.Sprintf("[%s:%s]", e.Window, e.Step))
if (*e.Window).Duration(1) > 0 || e.Step.Duration(1) > 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

Why this change?

@jiacai2050
Copy link
Owner

jiacai2050 commented Aug 30, 2022 •
edited 

指标{job="api"}

IMO, metricsql can't parse metric name with UTF-8.

I have filed an issue, but seems they have no interests

@jiacai2050
Copy link
Owner

You need to rebase with master, I have added CI in it.

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

@jiacai2050 jiacai2050 jiacai2050 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vegerot @jiacai2050