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

improve the quality of starlark docs by executing them as tests #8020

Merged
merged 3 commits into from
Aug 24, 2020

Conversation

ssoroka
Copy link
Contributor

@ssoroka ssoroka commented Aug 21, 2020

We can vastly improve the quality of the starlark examples by executing them, along with their documentation standards, as tests.

Using Example Input: and Example Output: as section header markers, we can parse the subsequent metrics out and use them as inputs to test the Starlark function, comparing the processed metrics to the example output, and failing if it does not match.

Example docs standards + function:

# Rename any tags using the mapping in the renames dict.
#
# Example Input:
# measurement,host=hostname lower=0,upper=100 1597255410000000000
#
# Example Output:
# measurement,host=hostname min=0,max=100 1597255410000000000

renames = {
    'lower': 'min',
    'upper': 'max',
}

def apply(metric):
    for k, v in metric.tags.items():
        if k in renames:
            metric.tags[renames[k]] = v
            metric.tags.pop(k)
    for k, v in metric.fields.items():
        if k in renames:
            metric.fields[renames[k]] = v
            metric.fields.pop(k)
    return metric

also the multi-line string format works, too:

# Filter metrics by value
'''
In this example we look at the `value` field of the metric.
If the value is zeor, we delete all the fields, effectively dropping the metric.

Example Input:
temperature sensor="001A0",value=111.48
temperature sensor="001B0",value=0.0

Example Output:
temperature sensor="001A0",value=111.48
'''

These get executed as tests, and if the lines are not parsable, the headers are missing, or the example output claimed is not generated, the test fails.

Example failures:

--- FAIL: TestAllScriptTestData (0.00s)
    --- FAIL: TestAllScriptTestData/testdata/rename.star (0.00s)
        starlark_test.go:2860: []telegraf.Metric
            --- expected
            +++ actual
              []*testutil.metricDiff{
              	&{
              		Measurement: "measurement",
              		Tags:        []*telegraf.Tag{&{Key: "host", Value: "hostname"}},
              		Fields: []*telegraf.Field{
            - 			&{Key: "max", Value: float64(100)},
              			&{
            - 				Key:   "min",
            + 				Key:   "lower",
              				Value: float64(0),
              			},
            + 			&{Key: "upper", Value: float64(100)},
              		},
              		Type: 3,
              		... // 1 ignored field
              	},
              }
    --- FAIL: TestAllScriptTestData/testdata/value_filter.star (0.00s)
        starlark_test.go:2820: 
            	Error Trace:	starlark_test.go:2820
            	            				starlark_test.go:2839
            	Error:      	Received unexpected error:
            	            	metric parse error: expected field at 1:23: "temperature sensor=001A0,value=111.48"
            	Test:       	TestAllScriptTestData/testdata/value_filter.star
            	Messages:   	Expected to be able to parse "Example Input:" metric, but found error
FAIL
FAIL	github.com/influxdata/telegraf/plugins/processors/starlark	0.350s
FAIL

results:

$ go test -v -run ^TestAllScriptTestData$ ./plugins/processors/starlark

=== RUN   TestAllScriptTestData
=== RUN   TestAllScriptTestData/testdata/number_logic.star
=== RUN   TestAllScriptTestData/testdata/pivot.star
=== RUN   TestAllScriptTestData/testdata/ratio.star
=== RUN   TestAllScriptTestData/testdata/rename.star
=== RUN   TestAllScriptTestData/testdata/scale.star
=== RUN   TestAllScriptTestData/testdata/value_filter.star
--- PASS: TestAllScriptTestData (0.00s)
    --- PASS: TestAllScriptTestData/testdata/number_logic.star (0.00s)
    --- PASS: TestAllScriptTestData/testdata/pivot.star (0.00s)
    --- PASS: TestAllScriptTestData/testdata/ratio.star (0.00s)
    --- PASS: TestAllScriptTestData/testdata/rename.star (0.00s)
    --- PASS: TestAllScriptTestData/testdata/scale.star (0.00s)
    --- PASS: TestAllScriptTestData/testdata/value_filter.star (0.00s)
PASS
ok  	github.com/influxdata/telegraf/plugins/processors/starlark	0.402s

@ssoroka ssoroka requested review from sjwang90 and russorat August 21, 2020 20:02
@ssoroka ssoroka added area/starlark docs Issues related to Telegraf documentation and configuration descriptions feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Aug 21, 2020
@ssoroka ssoroka self-assigned this Aug 21, 2020
@russorat
Copy link
Contributor

@ssoroka i really like the idea of testing these processors!

The first thing that comes to mind is if this in line with the way people test starlark code in other platforms? How would you test different scenarios for a processor? You could add a LP record for each scenario, but is there a way to document which line protocol line is testing what? I don't want to complicate things, but what does this look like when i have 10 different things i want to test in the starlark processor?

I wonder if the ability to name the tests might become important. Is it better to be explicit in the .star files with a test_TheValueIsZero() functions? Or instead of "Example Input", maybe the option to say something like "Example Input - This Tests something" and "Example Output"? Test Input/Output?

@ssoroka
Copy link
Contributor Author

ssoroka commented Aug 21, 2020

@russorat I like the way you think. We do already have existing processor tests in Go, but this does open the door to very simple tests that don't require Go. I don't think the Starlark language needs tests per-se, because that's covered elsewhere by the Starlark repo.

So what's left? I think these tests started as "let's make sure our implementation works", but going forward they're more useful as a user-facing tool, explaining how one might use Starlark to solve various problems. From that perspective, I don't think we should complicate them too much. it's less about stress testing the python function and more about ensuring it works as advertised. A user should be able to copy the function and examples and get the same result.

Given this change, I've been thinking about renaming the testdata folder to examples, as that's really the focus going forward.

Copy link
Contributor

@sjwang90 sjwang90 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing all my wrong LP syntax!

Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

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

good idea

@ssoroka ssoroka merged commit bbc2aa6 into master Aug 24, 2020
@ssoroka ssoroka deleted the docs-quality branch August 24, 2020 15:35
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/starlark docs Issues related to Telegraf documentation and configuration descriptions feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants