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.

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

Implement si and binary system for legendValue #490

Merged
merged 7 commits into from
Sep 13, 2023
Merged

Implement si and binary system for legendValue #490

merged 7 commits into from
Sep 13, 2023

Conversation

spacefreak86
Copy link
Contributor

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
deniszh previously approved these changes May 23, 2023
Copy link
Contributor

@deniszh deniszh left a comment

Choose a reason for hiding this comment

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

LGTM

@deniszh
Copy link
Contributor

deniszh commented Sep 4, 2023

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

Thanks!

@spacefreak86
Copy link
Contributor Author

Hi @deniszh
Rebase is done.

@deniszh
Copy link
Contributor

deniszh commented Sep 6, 2023

@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
Copy link
Contributor Author

@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
Copy link
Contributor

deniszh commented Sep 7, 2023

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
Copy link
Contributor Author

spacefreak86 commented Sep 7, 2023

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

@spacefreak86
Copy link
Contributor Author

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
Copy link
Contributor

deniszh commented Sep 11, 2023

@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
Copy link
Contributor Author

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

@deniszh deniszh merged commit 0aec9ab into bookingcom:master Sep 13, 2023
3 checks passed
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.

3 participants