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

k6runner: add check metadata and type to remote runner requests #928

Merged
merged 3 commits into from
Oct 22, 2024
+478 −327
Merged

k6runner: add check metadata and type to remote runner requests #928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17918df
k6runner: add check metadata and type to remote runner requests
roobre Oct 7, 2024
970436f
prober/browser,multihttp,scripted: fill CheckInfo
roobre Oct 7, 2024
4fb1659
make probers receive a model.Check instead of prober.Check
roobre Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions internal/k6runner/k6runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strings"

"github.com/go-logfmt/logfmt"
smmmodel "github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/logger"
"github.com/prometheus/client_golang/prometheus"
dto "github.com/prometheus/client_model/go"
Expand All @@ -20,11 +21,18 @@ import (

// Script is a k6 script that a runner is able to run, with some added instructions for that runner to act on.
type Script struct {
Script []byte `json:"script"`
// Script is the blob of bytes that is to be run.
Script []byte `json:"script"`
// Settings is a common representation of the fields common to all implementation-specific check settings that the
// runners are interested about.
Settings Settings `json:"settings"`
// TODO: Add Metadata and Features.
// CheckInfo holds information about the SM check that triggered this script.
CheckInfo CheckInfo `json:"check"`
// TODO: Add features.
}

// Settings is a common representation of the fields common to all implementation-specific check settings that the
// runners are interested about.
type Settings struct {
// Timeout for k6 run, in milliseconds. This value is a configuration value for remote runners, which will instruct
// them to return an error if the operation takes longer than this time to complete. Clients should expect that
Expand All @@ -33,6 +41,32 @@ type Settings struct {
Timeout int64 `json:"timeout"`
}

// CheckInfo holds information about the SM check that triggered this script.
type CheckInfo struct {
// Type is the string representation of the check type this script belongs to (browser, scripted, multihttp, etc.)
Type string `json:"type"`
// Metadata is a collection of key/value pairs containing information about this check, such as check and tenant ID.
// It is loosely typed on purpose: Metadata should only be used for informational properties that will make its way
// into telemetry, and not for making decision on it.
Metadata map[string]any `json:"metadata"`
}
Comment on lines +44 to +52
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where I'd like to have more reviewer attention, as these changes are hard to revert.

I chose to separate actionable information (Type), which will be used downstream to make decisions, from non-actionable data that will be basically used to enrich logging. I think this makes the relevant information "strongly typed", if that makes sense. Looking forward to your thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Metadata should only be used for informational properties that will make its way into telemetry

That's kind of the reason for using map[string]string instead of map[string]any.

For the purposes of info metrics, you are dealing with strings. Preserving the type doesn't gain you much.

Type is the string representation of the check type this script belongs to (browser, scripted, multihttp, etc.)

The only strong thing about the type here is that it requires it to be a) a field with a specific name; b) a string. Other than that, it's a free-form string, which defeats the strong type argument.

Note that you can unmarshal a map[string]any as anything you want on the other side. What I mean by that is that on the receiving side you can do e.g.

type CheckInfo {
   Type string `json:"type"`
   TenantId int64 `json:"tenantId"`
   RegionId int64 `json:"regionId"`
}

What I mean by this is that having a map on the producer side doesn't prevent you from having a well defined struct on the client side. Without some kind of validation, just having a field is not enough to force users of this code to actually set a type.

Unless you introduce actual coupling between the agent and the runner, which is not going to happen, this is all in internal/, the "strong type" argument is not very strong. I'm not saying it's wrong, just that it's not giving you what you say it gives you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's kind of the reason for using map[string]string instead of map[string]any.

I'm fine-ish with map[string]string for metadata. I leaned towards any because that will show integers as integers in the runner logs, instead of strings. If at some point we want to correlate logs, I think it would be nice that what is logged as an int in one place is also logged as an int on another.

Unless you introduce actual coupling between the agent and the runner, which is not going to happen, this is all in internal/, the "strong type" argument is not very strong. I'm not saying it's wrong, just that it's not giving you what you say it gives you.

Practically it's giving nothing, but I think the separation conveys meaning to readers. They will notice that particular field is special and think twice before adding or removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'm good with that, I just wanted to make sure the expectations are correct.

Do note that JSON numbers are super weird, because the JS in JSON stands for JavaScript, and JavaScript has no integers, only floats.

JSON mixed with Go is even weirder, because, when dealing with any, Go says "if JSON numbers are floats then I will decode them into float64 because that is what makes sense". You can override that using UseNumber (which requires using a json.Decoder not just json.Unmarshal) plus asking nicely for the specific conversion you want.

Do note this has an implication when marshaling elements from map[string]any into JSON via zerolog: the JSON decoder placed a float64 in the map's value, so logger.Any(...) sees a float, and it will log it out as a float. If you go thru the json.Number route, I think that should do the right thing (because json.Number's underlying type is string 😆).


// CheckInfoFromSM returns a CheckInfo from the information of the given SM check.
func CheckInfoFromSM(smc smmmodel.Check) CheckInfo {
ci := CheckInfo{
Metadata: map[string]any{},
}

ci.Type = smc.Type().String()
ci.Metadata["id"] = smc.Id
ci.Metadata["tenantID"] = smc.TenantId
ci.Metadata["regionID"] = smc.RegionId
ci.Metadata["created"] = smc.Created
ci.Metadata["modified"] = smc.Modified

return ci
}

// ErrNoTimeout is returned by [Runner] implementations if the supplied script has a timeout of zero.
var ErrNoTimeout = errors.New("check has no timeout")

Expand Down
30 changes: 30 additions & 0 deletions internal/k6runner/k6runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ import (
"strings"
"testing"

"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/logger"
"github.com/grafana/synthetic-monitoring-agent/internal/testhelper"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
"github.com/prometheus/client_golang/prometheus"
dto "github.com/prometheus/client_model/go"
"github.com/prometheus/common/expfmt"
Expand Down Expand Up @@ -93,6 +95,34 @@ func TestScriptRun(t *testing.T) {
require.True(t, success)
}

func TestCheckInfoFromSM(t *testing.T) {
t.Parallel()

check := model.Check{
RegionId: 4,
Check: sm.Check{
Id: 69,
TenantId: 1234,
Created: 1234.5,
Modified: 12345.6,
Settings: sm.CheckSettings{
Browser: &sm.BrowserSettings{}, // Make it non-nil so type is Browser.
},
},
}

ci := CheckInfoFromSM(check)

require.Equal(t, sm.CheckTypeBrowser.String(), ci.Type)
require.Equal(t, map[string]any{
"id": check.Id,
"tenantID": check.TenantId,
"regionID": check.RegionId,
"created": check.Created,
"modified": check.Modified,
}, ci.Metadata)
}

type testRunner struct {
metrics []byte
logs []byte
Expand Down
5 changes: 3 additions & 2 deletions internal/prober/browser/browser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"

"github.com/grafana/synthetic-monitoring-agent/internal/k6runner"
"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/logger"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
"github.com/prometheus/client_golang/prometheus"
Expand All @@ -26,7 +27,7 @@ type Prober struct {
processor *k6runner.Processor
}

func NewProber(ctx context.Context, check sm.Check, logger zerolog.Logger, runner k6runner.Runner) (Prober, error) {
func NewProber(ctx context.Context, check model.Check, logger zerolog.Logger, runner k6runner.Runner) (Prober, error) {
var p Prober

if check.Settings.Browser == nil {
Expand All @@ -40,7 +41,7 @@ func NewProber(ctx context.Context, check sm.Check, logger zerolog.Logger, runne
Settings: k6runner.Settings{
Timeout: check.Timeout,
},
// TODO: Add metadata & features here.
CheckInfo: k6runner.CheckInfoFromSM(check),
},
}

Expand Down
39 changes: 22 additions & 17 deletions internal/prober/browser/browser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"time"

"github.com/grafana/synthetic-monitoring-agent/internal/k6runner"
"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/testhelper"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
"github.com/rs/zerolog"
Expand All @@ -19,33 +20,37 @@ func TestNewProber(t *testing.T) {
logger := zerolog.New(zerolog.NewTestWriter(t))

testcases := map[string]struct {
check sm.Check
check model.Check
expectFailure bool
}{
"valid": {
expectFailure: false,
check: sm.Check{
Target: "http://www.example.org",
Job: "test",
Frequency: 10 * 1000,
Timeout: 10 * 1000,
Probes: []int64{1},
Settings: sm.CheckSettings{
Browser: &sm.BrowserSettings{
Script: []byte("// test"),
check: model.Check{
Check: sm.Check{
Target: "http://www.example.org",
Job: "test",
Frequency: 10 * 1000,
Timeout: 10 * 1000,
Probes: []int64{1},
Settings: sm.CheckSettings{
Browser: &sm.BrowserSettings{
Script: []byte("// test"),
},
},
},
},
},
"invalid": {
expectFailure: true,
check: sm.Check{
Target: "http://www.example.org",
Job: "test",
Frequency: 10 * 1000,
Timeout: 10 * 1000,
Probes: []int64{1},
Settings: sm.CheckSettings{},
check: model.Check{
Check: sm.Check{
Target: "http://www.example.org",
Job: "test",
Frequency: 10 * 1000,
Timeout: 10 * 1000,
Probes: []int64{1},
Settings: sm.CheckSettings{},
},
},
},
}
Expand Down
5 changes: 3 additions & 2 deletions internal/prober/dns/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"
"time"

"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/dns/internal/bbe/config"
bbeprober "github.com/grafana/synthetic-monitoring-agent/internal/prober/dns/internal/bbe/prober"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/logger"
Expand All @@ -21,7 +22,7 @@ type Prober struct {
experimental bool
}

func NewProber(check sm.Check) (Prober, error) {
func NewProber(check model.Check) (Prober, error) {
if check.Settings.Dns == nil {
return Prober{}, errUnsupportedCheck
}
Expand All @@ -35,7 +36,7 @@ func NewProber(check sm.Check) (Prober, error) {
}, nil
}

func NewExperimentalProber(check sm.Check) (Prober, error) {
func NewExperimentalProber(check model.Check) (Prober, error) {
p, err := NewProber(check)
if err != nil {
return p, err
Expand Down
37 changes: 21 additions & 16 deletions internal/prober/dns/dns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"time"

"github.com/go-kit/log"
"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/dns/internal/bbe/config"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
"github.com/miekg/dns"
Expand All @@ -27,17 +28,17 @@ func TestName(t *testing.T) {

func TestNewProber(t *testing.T) {
testcases := map[string]struct {
input sm.Check
input model.Check
expected Prober
ExpectError bool
}{
"default": {
input: sm.Check{
input: model.Check{Check: sm.Check{
Target: "www.grafana.com",
Settings: sm.CheckSettings{
Dns: &sm.DnsSettings{},
},
},
}},
expected: Prober{
config: config.Module{
Prober: "dns",
Expand All @@ -55,10 +56,12 @@ func TestNewProber(t *testing.T) {
ExpectError: false,
},
"no-settings": {
input: sm.Check{
Target: "www.grafana.com",
Settings: sm.CheckSettings{
Dns: nil,
input: model.Check{
Check: sm.Check{
Target: "www.grafana.com",
Settings: sm.CheckSettings{
Dns: nil,
},
},
},
expected: Prober{},
Expand Down Expand Up @@ -221,15 +224,17 @@ func TestProberRetries(t *testing.T) {
}
}()

p, err := NewExperimentalProber(sm.Check{
Target: "www.grafana.com",
Timeout: 20000,
Settings: sm.CheckSettings{
Dns: &sm.DnsSettings{
Server: l.LocalAddr().String(),
RecordType: sm.DnsRecordType_A,
Protocol: sm.DnsProtocol_UDP,
IpVersion: sm.IpVersion_V4,
p, err := NewExperimentalProber(model.Check{
Check: sm.Check{
Target: "www.grafana.com",
Timeout: 20000,
Settings: sm.CheckSettings{
Dns: &sm.DnsSettings{
Server: l.LocalAddr().String(),
RecordType: sm.DnsRecordType_A,
Protocol: sm.DnsProtocol_UDP,
IpVersion: sm.IpVersion_V4,
},
},
},
})
Expand Down
3 changes: 2 additions & 1 deletion internal/prober/grpc/grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"time"

"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/logger"
"github.com/grafana/synthetic-monitoring-agent/internal/tls"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
Expand All @@ -20,7 +21,7 @@ type Prober struct {
config config.Module
}

func NewProber(ctx context.Context, check sm.Check, logger zerolog.Logger) (Prober, error) {
func NewProber(ctx context.Context, check model.Check, logger zerolog.Logger) (Prober, error) {
if check.Settings.Grpc == nil {
return Prober{}, errUnsupportedCheck
}
Expand Down
23 changes: 14 additions & 9 deletions internal/prober/grpc/grpc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"io"
"testing"

"github.com/grafana/synthetic-monitoring-agent/internal/model"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
"github.com/prometheus/blackbox_exporter/config"
promcfg "github.com/prometheus/common/config"
Expand All @@ -19,15 +20,17 @@ func TestName(t *testing.T) {

func TestNewProber(t *testing.T) {
testcases := map[string]struct {
input sm.Check
input model.Check
expected Prober
ExpectError bool
}{
"default": {
input: sm.Check{
Target: "www.grafana.com",
Settings: sm.CheckSettings{
Grpc: &sm.GrpcSettings{},
input: model.Check{
Check: sm.Check{
Target: "www.grafana.com",
Settings: sm.CheckSettings{
Grpc: &sm.GrpcSettings{},
},
},
},
expected: Prober{
Expand All @@ -42,10 +45,12 @@ func TestNewProber(t *testing.T) {
},
},
"no-settings": {
input: sm.Check{
Target: "www.grafana.com",
Settings: sm.CheckSettings{
Grpc: nil,
input: model.Check{
Check: sm.Check{
Target: "www.grafana.com",
Settings: sm.CheckSettings{
Grpc: nil,
},
},
},
ExpectError: true,
Expand Down
5 changes: 3 additions & 2 deletions internal/prober/http/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strings"
"time"

"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/prober/logger"
"github.com/grafana/synthetic-monitoring-agent/internal/tls"
"github.com/grafana/synthetic-monitoring-agent/internal/version"
Expand All @@ -29,13 +30,13 @@ type Prober struct {
cacheBustingQueryParamName string
}

func NewProber(ctx context.Context, check sm.Check, logger zerolog.Logger, reservedHeaders http.Header) (Prober, error) {
func NewProber(ctx context.Context, check model.Check, logger zerolog.Logger, reservedHeaders http.Header) (Prober, error) {
if check.Settings.Http == nil {
return Prober{}, errUnsupportedCheck
}

if len(reservedHeaders) > 0 {
augmentHttpHeaders(&check, reservedHeaders)
augmentHttpHeaders(&check.Check, reservedHeaders)
}

cfg, err := settingsToModule(ctx, check.Settings.Http, logger)
Expand Down
Loading