Feature: Add support for multiple lua scripts evaluation #704

nantiferov · 2022-10-07T22:03:03Z

No breaking changes, the same option and env var supports now multiple Lua scripts separated by comma

Closes #699

oliver006 · 2022-10-08T02:40:05Z

First impression: this looks good! Let me know when you're ready for a review and I'd be happy to help with moving this along.

No breaking changes, the same option and env var supports now multiple Lua scripts separated by comma Closes #699

coveralls · 2022-10-10T09:01:35Z

Pull Request Test Coverage Report for Build 1737

6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.004%) to 92.171%

Totals
Change from base Build 1732:	0.004%
Covered Lines:	1813
Relevant Lines:	1967

💛 - Coveralls

codecov · 2022-10-10T09:02:49Z

Codecov Report

Merging #704 (c228c51) into master (f66c1bf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head c228c51 differs from pull request most recent head 4ac5b88. Consider uploading reports for the commit 4ac5b88 to get more accurate results

@@           Coverage Diff           @@
##           master     #704   +/-   ##
=======================================
  Coverage   88.26%   88.27%           
=======================================
  Files          16       16           
  Lines        1960     1961    +1     
=======================================
+ Hits         1730     1731    +1     
  Misses        154      154           
  Partials       76       76

Impacted Files	Coverage Δ
exporter/exporter.go	`91.60% <100.00%> (+0.01%)`	⬆️
exporter/lua.go	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

nantiferov · 2022-10-10T09:07:39Z

Let me know when you're ready for a review and I'd be happy to help with moving this along

Morning,
I fixed tests now and should be good to go.

I tested quickly new version with redis manually, no issues noticed.

oliver006 · 2022-10-12T00:17:10Z

exporter/exporter.go

-		if err := e.extractLuaScriptMetrics(ch, c); err != nil {
-			return err
+		for _, script := range e.options.LuaScript {
+			if err := e.extractLuaScriptMetrics(ch, c, script); err != nil {


My apologies, I should have caught this during the initial, cursory review but missed it.
I think you should continue here when an error occurs and 1) log the error to the console and b) record a gauge metric with success==1/error==0 with the script name as a label.

To do that you'll need to have the label so change LuaScript in the exporter struct to be an array of Filename string & Script []byte so you have the filename available.

You should probably also record that filename label with the lua script results in lua.go:30 to avoid collisions of two scripts using the same variable name when returning results and for added info where a value is coming from.

Hi,
Thank you for review. I will try to update it this week.

But are you sure that we want to start exporter even if file is not found?

log the error to the console

It's already doing it failing on first not accessible file FATA[0000] Error loading script file test1.lua err: open test1.lua: no such file or directory

You should probably also record that filename label with the lua script results in lua.go:30 to avoid collisions of two scripts using the same variable name when returning results and for added info where a value is coming from.

Yep, this sounds reasonable.

You're right, this should fail fast on startup if a file is not present and the metric can record if executing the lua script succeeded or not.

Hi @oliver006
Merry Christmas and a happy new year!🎄

I managed to check my PR during holidays and I think I implemented all we discussed.

Thanks for all your effort here!
I'll merge this as soon as the CI server runs (it sometimes gets backed up)

Any merry Christmas and a happy, healthy new year to you too!

Hi @oliver006
It seems CI has some issues?

Yeah, the CI has issues, it's pretty annoying. Let me find time the next few days and migrate it to a new CI instance. In the meantime, I'll merge the PR.

Finally got around to fixing the CI, your feature is released now as https://github.com/oliver006/redis_exporter/releases/tag/v1.46.0

…-evaulation

oliver006 · 2022-12-09T02:50:26Z

Quick ping here to see if you're still working on this, would love to see this land.

nantiferov · 2022-12-09T07:31:33Z

Hi,
Sorry, don't have much free time in the end of the year.
So far I plan to take a look at this somewhere on December holidays.

oliver006 · 2022-12-09T13:02:32Z

No worries, I get it, end of the year is hectic, I just wanted to check in. Take your time and ping me when you're ready for review.

To monitor lua script evaluation: 0-error 1-ok 2-no results

nantiferov · 2022-12-26T15:35:03Z

exporter/exporter.go

@@ -374,7 +374,8 @@ func NewRedisExporter(redisURI string, opts Options) (*Exporter, error) {
 		"master_link_up":                               {txt: "Master link status on Redis slave", lbls: []string{"master_host", "master_port"}},
 		"master_sync_in_progress":                      {txt: "Master sync in progress", lbls: []string{"master_host", "master_port"}},
 		"number_of_distinct_key_groups":                {txt: `Number of distinct key groups`, lbls: []string{"db"}},
-		"script_values":                                {txt: "Values returned by the collect script", lbls: []string{"key"}},
+		"script_result":                                {txt: "Result of the collect script evaluation", lbls: []string{"filename"}},


As we discussed, I added new metric script_result, which will indicate if evaluation of Lua failed.
So far I set 0 - fail, 1 - ok, 2 - empty results.

nantiferov · 2022-12-26T15:35:42Z

exporter/lua_test.go

-			ExpectedKeys: 3,
-			Wants:        []string{`test_exporter_last_scrape_error{err=""} 0`, `test_script_values{key="a"} 11`, `test_script_values{key="b"} 12`},
+			ExpectedKeys: 4,
+			Wants:        []string{`test_exporter_last_scrape_error{err=""} 0`, `test_script_values{filename="test.lua",key="a"} 11`, `test_script_values{filename="test.lua",key="b"} 12`, `test_script_values{filename="test.lua",key="c"} 13`, `test_script_result{filename="test.lua"} 1`},


I noticed that tests were missing one result, so I added it

`test_script_values{filename="test.lua",key="c"} 13`,

nantiferov · 2022-12-26T15:36:28Z

exporter/lua_test.go

 			ExpectedError: true,
-			Wants:         []string{`test_exporter_last_scrape_error{err="ERR Error compiling script`},
+			Wants:         []string{`test_exporter_last_scrape_error{err="ERR Error compiling script`, `test_script_result{filename="test.lua"} 0`},


I added check of new metric script_result to borked1, borked2 tests

oliver006

Nice work, thanks for finishing this!

oliver006 · 2022-12-26T17:12:56Z

exporter/http_test.go

-				LuaScript: []byte(`return {"a", "11", "b", "12", "c", "13"}`),
-				Registry:  prometheus.NewRegistry(),
+				LuaScript: map[string][]byte{
+					"test.lua": []byte(`return {"a", "11", "b", "12", "c", "13"}`),


Feature: Add support for multiple lua scripts evaulation

9936924

No breaking changes, the same option and env var supports now multiple Lua scripts separated by comma Closes #699

nantiferov marked this pull request as ready for review October 10, 2022 09:06

oliver006 reviewed Oct 12, 2022

View reviewed changes

Merge branch 'oliver006:master' into support-for-multiple-lua-scripts…

c228c51

…-evaulation

oliver006 changed the title ~~Feature: Add support for multiple lua scripts evaulation~~ Feature: Add support for multiple lua scripts evaluation Dec 9, 2022

nantiferov added 3 commits December 26, 2022 15:53

Switch to use string:byte map instead of [][]byte, add filename label

67b0707

Add script_result metric

723ffaf

To monitor lua script evaluation: 0-error 1-ok 2-no results

Fix tests for new label

4ac5b88

nantiferov commented Dec 26, 2022

View reviewed changes

oliver006 approved these changes Dec 26, 2022

View reviewed changes

oliver006 merged commit 48b92fc into oliver006:master Jan 20, 2023

This was referenced Jun 2, 2023

Impossible to use multiple Lua scripts with exporter #699

Closed

Update docs on multiple lua scripts evaluation #804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add support for multiple lua scripts evaluation #704

Feature: Add support for multiple lua scripts evaluation #704

nantiferov commented Oct 7, 2022

oliver006 commented Oct 8, 2022

coveralls commented Oct 10, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

nantiferov commented Oct 10, 2022

oliver006 Oct 12, 2022

nantiferov Oct 17, 2022 •

edited

Loading

oliver006 Oct 19, 2022

nantiferov Dec 26, 2022

oliver006 Dec 26, 2022

nantiferov Jan 20, 2023

oliver006 Jan 20, 2023

oliver006 Jan 29, 2023

oliver006 commented Dec 9, 2022

nantiferov commented Dec 9, 2022

oliver006 commented Dec 9, 2022

nantiferov Dec 26, 2022

nantiferov Dec 26, 2022

nantiferov Dec 26, 2022

oliver006 left a comment

oliver006 Dec 26, 2022

Feature: Add support for multiple lua scripts evaluation #704

Feature: Add support for multiple lua scripts evaluation #704

Conversation

nantiferov commented Oct 7, 2022

oliver006 commented Oct 8, 2022

coveralls commented Oct 10, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1737

💛 - Coveralls

codecov bot commented Oct 10, 2022 • edited Loading

Codecov Report

nantiferov commented Oct 10, 2022

Choose a reason for hiding this comment

nantiferov Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver006 commented Dec 9, 2022

nantiferov commented Dec 9, 2022

oliver006 commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver006 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 10, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

nantiferov Oct 17, 2022 •

edited

Loading