Skip to content

Commit eca0acc

Browse files
bobheadxibobheadxi
authored
telemetry: support float64 attributes (sourcegraph#58643)
In Javascript, there is no way to easily determine if a number is an integer or a float, as JS numbers are [floats](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number). This makes `value: Int!` hard to use, because there is no way to ask callers to provide metadata in integer values only. To support clients (typically in JS/TS) well, we need to accept `value: Float!` - however, this is a breaking change: ``` In element #0: In field "parameters": In field "metadata": In element #1: In field "value": Expected type "Float", found 0. (line 3, column 28) ``` To make this change smoothly, we accept `value: JSONValue` in the GraphQL input and validate that it is a numeric value in the backend - this allows clients to provide both integers and floats. For events export, the change is simpler - we add a new field `metadata = 5`, and mark the existing metadata field as deprecated (`legacy_metadata`). In proto, only the field index matters, so the rename is safe, and ensures there is no disruption for data going to BigQuery, since we can continue to use `metadata` as the JSON field and simply drop `legacy_metadata` entirely. For existing clients, out-of-band telemetry clients (namely Cody) will need to be updated to round values to integers unless interacting with a more recent Sourcegraph version, e.g. sourcegraph/cody#1913 (comment) ## Test plan - Unit tests validating GraphQL schema works as expected, including back-compat - Unit tests on telemetry gateway field migration - Eixsting tests
1 parent de2290e commit eca0acc

File tree

22 files changed

+363
-184
lines changed

22 files changed

+363
-184
lines changed

‎cmd/frontend/graphqlbackend/telemetry.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ type TelemetryEventParametersInput struct {
4040
}
4141

4242
type TelemetryEventMetadataInput struct {
43-
Key string `json:"key"`
44-
Value int32 `json:"value"`
43+
Key string `json:"key"`
44+
Value JSONValue `json:"value"`
4545
}
4646

4747
type TelemetryEventBillingMetadataInput struct {

‎cmd/frontend/graphqlbackend/telemetry.graphql

+5-1
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,12 @@ input TelemetryEventMetadataInput {
119119
"""
120120
Numeric value associated with the key. Enforcing numeric values eliminates
121121
risks of accidentally shipping sensitive or private data.
122+
123+
The value type in the schema is JSONValue for flexibility, but we ONLY
124+
accept numeric values (integers and floats) - any other value will be
125+
rejected.
122126
"""
123-
value: Int!
127+
value: JSONValue!
124128
}
125129

126130
"""

‎cmd/frontend/graphqlbackend/telemetry_test.go

+45-21
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func TestTelemetryRecordEvents(t *testing.T) {
5959
},
6060
},
6161
{
62-
name: "object privateMetadata",
62+
name: "metadata with standard object privateMetadata",
6363
gqlEventsInput: `
6464
{
6565
feature: "cody.fixup"
@@ -96,6 +96,11 @@ func TestTelemetryRecordEvents(t *testing.T) {
9696
// Sanity check strucpb marshalling used in cmd/frontend/internal/telemetry/resolvers
9797
_, err := structpb.NewStruct(v)
9898
require.NoError(t, err)
99+
100+
md := *gotEvents[0].Parameters.Metadata
101+
require.Len(t, md, 2)
102+
assert.Equal(t, int32(1), md[0].Value.Value)
103+
assert.Equal(t, int32(0), md[1].Value.Value)
99104
},
100105
},
101106
{
@@ -110,16 +115,6 @@ func TestTelemetryRecordEvents(t *testing.T) {
110115
}
111116
parameters: {
112117
version: 0
113-
metadata: [
114-
{
115-
key: "contextSelection",
116-
value: 1
117-
},
118-
{
119-
key: "chatPredictions",
120-
value: 0
121-
},
122-
]
123118
privateMetadata: "some value"
124119
}
125120
}
@@ -150,16 +145,6 @@ func TestTelemetryRecordEvents(t *testing.T) {
150145
}
151146
parameters: {
152147
version: 0
153-
metadata: [
154-
{
155-
key: "contextSelection",
156-
value: 1
157-
},
158-
{
159-
key: "chatPredictions",
160-
value: 0
161-
},
162-
]
163148
privateMetadata: 12
164149
}
165150
}
@@ -178,6 +163,45 @@ func TestTelemetryRecordEvents(t *testing.T) {
178163
require.NoError(t, err)
179164
},
180165
},
166+
{
167+
name: "different numeric values in metadata",
168+
gqlEventsInput: `
169+
{
170+
feature: "cody.fixup"
171+
action: "applied"
172+
source: {
173+
client: "VSCode.Cody",
174+
clientVersion: "0.14.1"
175+
}
176+
parameters: {
177+
version: 0
178+
metadata: [
179+
{
180+
key: "contextSelection",
181+
value: 1
182+
},
183+
{
184+
key: "chatPredictions",
185+
value: 0
186+
},
187+
{
188+
key: "fooBar",
189+
value: 3.14
190+
},
191+
]
192+
}
193+
}
194+
`,
195+
assert: func(t *testing.T, gotEvents []TelemetryEventInput) {
196+
// Check PrivateMetadata
197+
require.Len(t, gotEvents, 1)
198+
md := *gotEvents[0].Parameters.Metadata
199+
require.Len(t, md, 3)
200+
assert.Equal(t, int32(1), md[0].Value.Value)
201+
assert.Equal(t, int32(0), md[1].Value.Value)
202+
assert.Equal(t, 3.14, md[2].Value.Value)
203+
},
204+
},
181205
} {
182206
t.Run(tc.name, func(t *testing.T) {
183207
mockResolver := &mockTelemetryResolver{}

‎cmd/frontend/graphqlbackend/testing.go

+7
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,13 @@ func checkErrors(t *testing.T, want, got []*gqlerrors.QueryError) {
130130

131131
// Compare without caring about the concrete type of the error returned
132132
if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(gqlerrors.QueryError{}, "ResolverError", "Err")); diff != "" {
133+
// diff truncates the error messages, so dump the full error messages
134+
// in t.Log for inspection with 'go test -v'.
135+
t.Log("Full errors:\n")
136+
for _, e := range got {
137+
t.Logf("- %+v\n", e)
138+
}
139+
// Fail the test with the diff as a summary
133140
t.Fatal(diff)
134141
}
135142
}

‎cmd/frontend/internal/telemetry/resolvers/telemetrygateway.go

+29-11
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
"github.com/sourcegraph/sourcegraph/lib/errors"
1313
)
1414

15+
// newTelemetryGatewayEvents converts GraphQL telemetry input to the Telemetry
16+
// Gateway wire format.
1517
func newTelemetryGatewayEvents(
1618
ctx context.Context,
1719
now time.Time,
@@ -34,6 +36,31 @@ func newTelemetryGatewayEvents(
3436
event.Interaction.InteractionId = gqlEvent.Parameters.InteractionID
3537
}
3638

39+
// Parse metadata
40+
var metadata map[string]float64
41+
if gqlEvent.Parameters.Metadata != nil && len(*gqlEvent.Parameters.Metadata) != 0 {
42+
metadata = make(map[string]float64, len(*gqlEvent.Parameters.Metadata))
43+
for _, kv := range *gqlEvent.Parameters.Metadata {
44+
switch v := kv.Value.Value.(type) {
45+
case int:
46+
metadata[kv.Key] = float64(v)
47+
case int8:
48+
metadata[kv.Key] = float64(v)
49+
case int32:
50+
metadata[kv.Key] = float64(v)
51+
case int64:
52+
metadata[kv.Key] = float64(v)
53+
case float32:
54+
metadata[kv.Key] = float64(v)
55+
case float64:
56+
metadata[kv.Key] = v
57+
default:
58+
return gatewayEvents,
59+
errors.Newf("metadata %q has invalid value type %T", kv.Key, v)
60+
}
61+
}
62+
}
63+
3764
// Parse private metadata
3865
var privateMetadata *structpb.Struct
3966
if gqlEvent.Parameters.PrivateMetadata != nil {
@@ -60,17 +87,8 @@ func newTelemetryGatewayEvents(
6087

6188
// Configure parameters
6289
event.Parameters = &telemetrygatewayv1.EventParameters{
63-
Version: gqlEvent.Parameters.Version,
64-
Metadata: func() map[string]int64 {
65-
if gqlEvent.Parameters.Metadata == nil || len(*gqlEvent.Parameters.Metadata) == 0 {
66-
return nil
67-
}
68-
metadata := make(map[string]int64, len(*gqlEvent.Parameters.Metadata))
69-
for _, kv := range *gqlEvent.Parameters.Metadata {
70-
metadata[kv.Key] = int64(kv.Value)
71-
}
72-
return metadata
73-
}(),
90+
Version: gqlEvent.Parameters.Version,
91+
Metadata: metadata,
7492
PrivateMetadata: privateMetadata,
7593
BillingMetadata: func() *telemetrygatewayv1.EventBillingMetadata {
7694
if gqlEvent.Parameters.BillingMetadata == nil {

‎cmd/frontend/internal/telemetry/resolvers/telemetrygateway_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
104104
Metadata: &[]graphqlbackend.TelemetryEventMetadataInput{
105105
{
106106
Key: "metadata",
107-
Value: 123,
107+
Value: graphqlbackend.JSONValue{Value: 123},
108108
},
109109
},
110110
PrivateMetadata: &graphqlbackend.JSONValue{
@@ -126,7 +126,7 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
126126
"product": "Product"
127127
},
128128
"metadata": {
129-
"metadata": "123"
129+
"metadata": 123
130130
},
131131
"privateMetadata": {
132132
"private": "super-sensitive"

‎cmd/telemetry-gateway/internal/server/BUILD.bazel

+6-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ go_library(
55
name = "server",
66
srcs = [
77
"metrics.go",
8+
"migrate_events.go",
89
"publish_events.go",
910
"server.go",
1011
],
@@ -29,10 +30,14 @@ go_library(
2930

3031
go_test(
3132
name = "server_test",
32-
srcs = ["publish_events_test.go"],
33+
srcs = [
34+
"migrate_events_test.go",
35+
"publish_events_test.go",
36+
],
3337
embed = [":server"],
3438
deps = [
3539
"//cmd/telemetry-gateway/internal/events",
40+
"//internal/telemetrygateway/v1:telemetrygateway",
3641
"@com_github_hexops_autogold_v2//:autogold",
3742
"@com_github_stretchr_testify//assert",
3843
],

‎cmd/telemetry-gateway/internal/server/migrate_events.go

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package server
2+
3+
import telemetrygatewayv1 "github.com/sourcegraph/sourcegraph/internal/telemetrygateway/v1"
4+
5+
// migrateEvents does an in-place migration of any legacy field usages to support
6+
// older exporters.
7+
func migrateEvents(events []*telemetrygatewayv1.Event) {
8+
for _, ev := range events {
9+
if ev.Parameters != nil && len(ev.Parameters.LegacyMetadata) > 0 {
10+
if ev.Parameters.Metadata == nil {
11+
ev.Parameters.Metadata = make(map[string]float64, len(ev.Parameters.LegacyMetadata))
12+
}
13+
for k, v := range ev.Parameters.LegacyMetadata {
14+
ev.Parameters.Metadata[k] = float64(v)
15+
}
16+
ev.Parameters.LegacyMetadata = nil // remove
17+
}
18+
}
19+
}

‎cmd/telemetry-gateway/internal/server/migrate_events_test.go

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package server
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
8+
telemetrygatewayv1 "github.com/sourcegraph/sourcegraph/internal/telemetrygateway/v1"
9+
)
10+
11+
func TestMigrateEvents(t *testing.T) {
12+
t.Run("legacy metadata format", func(t *testing.T) {
13+
events := []*telemetrygatewayv1.Event{
14+
{
15+
Parameters: &telemetrygatewayv1.EventParameters{
16+
LegacyMetadata: map[string]int64{
17+
"foo": 123,
18+
},
19+
},
20+
},
21+
{
22+
Parameters: &telemetrygatewayv1.EventParameters{
23+
LegacyMetadata: map[string]int64{
24+
"foo": 123,
25+
},
26+
Metadata: map[string]float64{
27+
"bar": 456,
28+
},
29+
},
30+
},
31+
}
32+
migrateEvents(events)
33+
34+
// first event
35+
assert.Equal(t, float64(123), events[0].Parameters.Metadata["foo"])
36+
assert.Nil(t, events[0].Parameters.LegacyMetadata)
37+
38+
// second event
39+
assert.Equal(t, float64(123), events[1].Parameters.Metadata["foo"])
40+
assert.Equal(t, float64(456), events[1].Parameters.Metadata["bar"])
41+
assert.Nil(t, events[1].Parameters.LegacyMetadata)
42+
})
43+
}

‎cmd/telemetry-gateway/internal/server/server.go

+3
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,9 @@ func (s *Server) RecordEvents(stream telemetrygatewayv1.TelemeteryGatewayService
118118
return status.Error(codes.InvalidArgument, "got events when metadata not yet received")
119119
}
120120

121+
// Handle legacy exporters
122+
migrateEvents(events)
123+
121124
// Publish events
122125
resp := handlePublishEvents(
123126
stream.Context(),

‎doc/dev/background-information/telemetry/protocol.md

+19-1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ This page contains generated documentation for telemetry event data that gets ex
2121
- [EventInteraction.Geolocation](#telemetrygateway-v1-EventInteraction-Geolocation)
2222
- [EventMarketingTracking](#telemetrygateway-v1-EventMarketingTracking)
2323
- [EventParameters](#telemetrygateway-v1-EventParameters)
24+
- [EventParameters.LegacyMetadataEntry](#telemetrygateway-v1-EventParameters-LegacyMetadataEntry)
2425
- [EventParameters.MetadataEntry](#telemetrygateway-v1-EventParameters-MetadataEntry)
2526
- [EventSource](#telemetrygateway-v1-EventSource)
2627
- [EventSource.Client](#telemetrygateway-v1-EventSource-Client)
@@ -185,6 +186,7 @@ Sourcegraph.com instance.
185186
| Field | Type | Label | Description |
186187
| ----- | ---- | ----- | ----------- |
187188
| version | [int32](#int32) | | <p>Version of the event parameters, used for indicating the "shape" of this</p><p>event's metadata, beginning at 0. Useful for denoting if the shape of</p><p>metadata has changed in any way.</p> |
189+
| legacy_metadata | [EventParameters.LegacyMetadataEntry](#telemetrygateway-v1-EventParameters-LegacyMetadataEntry) | repeated | <p>Legacy metadata format that only accepted int64 - use the new metadata</p><p>field instead, which accepts float values. Values sent through this proto</p><p>field will be merged into the new metadata attributes.</p><p>We don't use a [deprecated = true] tag because we use this field to handle</p><p>accepting exporters sending metadata in this format.</p> |
188190
| metadata | [EventParameters.MetadataEntry](#telemetrygateway-v1-EventParameters-MetadataEntry) | repeated | <p>Strictly typed metadata, restricted to integer values to avoid accidentally</p><p>exporting sensitive or private data.</p> |
189191
| private_metadata | [google.protobuf.Struct](#google-protobuf-Struct) | optional | <p>Additional potentially sensitive metadata - i.e. not restricted to integer</p><p>values.</p><p>🚨 SECURITY: This metadata is NOT exported from instances by default, as it</p><p>can contain arbitrarily-shaped data that may accidentally contain sensitive</p><p>or private contents.</p><p>This metadata is only exported on an allowlist basis based on terms of</p><p>use agreements and combinations of event feature and action, alongside</p><p>careful audit of callsites.</p> |
190192
| billing_metadata | [EventBillingMetadata](#telemetrygateway-v1-EventBillingMetadata) | optional | <p>Optional billing-related metadata.</p> |
@@ -194,6 +196,22 @@ Sourcegraph.com instance.
194196

195197

196198

199+
<a name="telemetrygateway-v1-EventParameters-LegacyMetadataEntry"></a>
200+
201+
### EventParameters.LegacyMetadataEntry
202+
203+
204+
205+
| Field | Type | Label | Description |
206+
| ----- | ---- | ----- | ----------- |
207+
| key | [string](#string) | | <p></p> |
208+
| value | [int64](#int64) | | <p></p> |
209+
210+
211+
212+
213+
214+
197215
<a name="telemetrygateway-v1-EventParameters-MetadataEntry"></a>
198216

199217
### EventParameters.MetadataEntry
@@ -203,7 +221,7 @@ Sourcegraph.com instance.
203221
| Field | Type | Label | Description |
204222
| ----- | ---- | ----- | ----------- |
205223
| key | [string](#string) | | <p></p> |
206-
| value | [int64](#int64) | | <p></p> |
224+
| value | [double](#double) | | <p></p> |
207225

208226

209227

‎internal/completions/client/observe.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func (o *observedClient) Stream(ctx context.Context, feature types.CompletionsFe
4444

4545
o.events.Record(ctx, "cody.completions", "stream", &telemetry.EventParameters{
4646
Metadata: telemetry.EventMetadata{
47-
"feature": int64(feature.ID()),
47+
"feature": float64(feature.ID()),
4848
},
4949
PrivateMetadata: map[string]any{
5050
"stop_reason": event.StopReason,
@@ -69,7 +69,7 @@ func (o *observedClient) Complete(ctx context.Context, feature types.Completions
6969

7070
defer o.events.Record(ctx, "cody.completions", "complete", &telemetry.EventParameters{
7171
Metadata: telemetry.EventMetadata{
72-
"feature": int64(feature.ID()),
72+
"feature": float64(feature.ID()),
7373
},
7474
})
7575

‎internal/database/telemetry_export_store_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ func TestTelemetryEventsExportQueueLifecycle(t *testing.T) {
102102
Action: "View",
103103
Timestamp: timestamppb.New(time.Date(2022, 11, 3, 1, 0, 0, 0, time.UTC)),
104104
Parameters: &telemetrygatewayv1.EventParameters{
105-
Metadata: map[string]int64{"public": 1},
105+
Metadata: map[string]float64{"public": 1},
106106
},
107107
}, {
108108
Id: "2",

0 commit comments

Comments
 (0)