search

bookingcom / carbonapi

High-performance Graphite frontend in Go
Other
81 stars 23 forks source link

Implement si and binary system for legendValue #490

Closed spacefreak86 closed 1 year ago

spacefreak86 commented 1 year ago

Sometimes it is useful to print values (current, min, max, average) in the graph legend in human readable form. Unfortunately, legendValue function of carbonapi is only able to print the raw values, because the systems "si" and "binary" are not implemented yet.

This PR implements the "si" and "binary" system for the legendValue function, it is currently running on my productive environment without any issues.

Please let me know if you have any questions or remarks.

deniszh commented 1 year ago

Hi @spacefreak86 Sorry for neglecting this :( Could you please rebase against master?

Thanks!

spacefreak86 commented 1 year ago

Hi @deniszh Rebase is done.

deniszh commented 1 year ago

@spacefreak86 : thank you! but tests are failing now, unfortunately. Not sure if test needs to be fixed or system support breaks something. Could you please check? Thank you!

spacefreak86 commented 1 year ago

@deniszh the problem is that I have changed the format in which legend values are printed on the graph.

The original format was like this: metric1 (sum: 15.000000) (avg: 3.000000)

I have changed it to this format (also used before by Graphite-Web): metric1 (sum: 15.000000, avg: 3.000000)

The new format saves some space on the graph and in my opinion it also looks nicer. What do you think?

Based on your answer, I will switch back to the original format or I will modify the test accordingly.

Thank you for looking into this.

deniszh commented 1 year ago

Hi @spacefreak86 Thank you for checking. I would prefer keeping graphite-web compatibility, even if change current implementation. If your change make it original graphite compatible - please keep it and change test. Thank you!

spacefreak86 commented 1 year ago

Hi @deniszh I have changed the test, it should pass now. Thanks for your fast feedback.

spacefreak86 commented 1 year ago

Hi @deniszh It seems that 2/3 CI tests passed. Unfortunately, I have no idea why the 1.21.x test failed. I think it has nothing to do with my change. Could you please take a look at it?

deniszh commented 1 year ago

@spacefreak86 : it's failed because of linter:

  Running [/home/runner/golangci-lint-1.54.2-linux-amd64/golangci-lint run --out-format=github-actions] in [] ...
  Error: File is not `gofmt`-ed with `-s` (gofmt)

  Error: issues found

It doesn't say which file, but just format both - 

gofmt -s -w pkg/expr/expr_test.go
gofmt -s -w pkg/expr/functions/legendValue/function.go

and submit changes.

spacefreak86 commented 1 year ago

@deniszh done, it was just two tabs on an empty line.