feat: New Metrics metadata

[chriswk]

is included in client registration and client metrics

[sighphyre]

The code looks cool but what happened to specVersion?

[nunogois]

Should started be configurable?

[chriswk]

Mostly for tests here

[chriswk]

I would've used #[cfg(test)] here, don't know what the same is in go.

[chriswk]

Good point, I've moved it into the test file where I used it instead.

[nunogois]

I still don't like that we're polluting our config options with something that shouldn't be user-configurable and I'm still not super convinced of its value. Maybe we can relax our tests instead?

[nunogois]

Something like this seems to work, but maybe there's a better way:

func clientDataMatcher() func(req *http.Request, ereq *gock.Request) (bool, error) {
	defaultStrategies := []string{
		"default",
		"applicationHostname",
		"gradualRolloutRandom",
		"gradualRolloutSessionId",
		"gradualRolloutUserId",
		"remoteAddress",
		"userWithId",
		"flexibleRollout",
	}
	return func(req *http.Request, ereq *gock.Request) (bool, error) {
		var data ClientData
		err := json.NewDecoder(req.Body).Decode(&data)
		if err != nil {
			return false, err
		}

		if data.Started.IsZero() {
			return false, nil
		}

		expectedData := ClientData{
			AppName:          mockAppName,
			InstanceID:       mockInstanceId,
			SDKVersion:       fmt.Sprintf("%s:%s", clientName, clientVersion),
			Strategies:       defaultStrategies,
			Interval:         0,
			PlatformVersion:  runtime.Version(),
			PlatformName:     "go",
			YggdrasilVersion: nil,
			SpecVersion:      specVersion,
		}

		return data.AppName == expectedData.AppName &&
			data.InstanceID == expectedData.InstanceID &&
			data.SDKVersion == expectedData.SDKVersion &&
			compareStringSlices(data.Strategies, expectedData.Strategies) &&
			data.Interval == expectedData.Interval &&
			data.PlatformVersion == expectedData.PlatformVersion &&
			data.PlatformName == expectedData.PlatformName &&
			data.YggdrasilVersion == expectedData.YggdrasilVersion &&
			data.SpecVersion == expectedData.SpecVersion, nil
	}
}

func compareStringSlices(a, b []string) bool {
	if len(a) != len(b) {
		return false
	}
	for i := range a {
		if a[i] != b[i] {
			return false
		}
	}
	return true
}

func TestMetrics_ClientDataIncludesNewMetadata(t *testing.T) {
	assert := assert.New(t)
	defer gock.OffAll()

	gock.New(mockerServer).
		Post("/client/register").
		AddMatcher(clientDataMatcher()).
		Reply(200)

	client, err := NewClient(
		WithUrl(mockerServer),
		WithMetricsInterval(50*time.Millisecond),
		WithAppName(mockAppName),
		WithInstanceId(mockInstanceId),
	)

	assert.Nil(err, "Client should open without a problem")

	time.Sleep(100 * time.Millisecond)

	err = client.Close()

	assert.Nil(err, "Client should close without errors")

	st.Expect(t, gock.IsDone(), true)
}

[chriswk]

Thanks. I added your suggestion

[chriswk]

And removed started from the options

[chriswk]

The code looks cool but what happened to specVersion?

Don't you come here with your facts 🥇 . Added specVersion...

[sighphyre]

Noice! LGTM

[nunogois]

I believe this can be reverted now, and we can set started: time.Now() like before.

[nunogois]

These are helper functions, not tests. Maybe we should move them to the top of the file or somewhere else that's not here?

[chriswk]

Why not here, they're used immediately after?

[nunogois]

Simply readability / convention / preference. I don't have strong feelings either way, and I understand the logic of keeping them near to where they're used, so I'm happy with keeping this.

[nunogois]

LGTM - Thank you for considering my comments 😄