Skip to content

Commit

Permalink
fix: Only set incoming user agent if not already present (#1366)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

Following up to the following PR, we should only set the incoming user
agent if the key does not already exist. This is useful in certain
scenarios when two Refinery's are connected together.

- #1358

## Short description of the changes
- Only set incoming user agent meta field if the event doesn't already
have a value for it
- Add unit test to verify behaviour
- Update existing tests to set custom user-agent and verify it's set
correctly
# Conflicts:
#	app/app_test.go
#	route/route_test.go
  • Loading branch information
MikeGoldsmith committed Oct 7, 2024
1 parent 0d88b9a commit 869da1a
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 16 deletions.
21 changes: 6 additions & 15 deletions app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"net/http"
"net/http/httptest"
"os"
"runtime"
"strconv"
"strings"
"sync"
Expand All @@ -28,7 +27,6 @@ import (

"github.com/honeycombio/libhoney-go"
"github.com/honeycombio/libhoney-go/transmission"
"github.com/honeycombio/libhoney-go/version"
"github.com/honeycombio/refinery/collect"
"github.com/honeycombio/refinery/config"
"github.com/honeycombio/refinery/internal/health"
Expand Down Expand Up @@ -217,6 +215,7 @@ func newStartedApp(
}

func post(t testing.TB, req *http.Request) {
req.Header.Set("User-Agent", "Test-Client")
resp, err := httpClient.Do(req)
assert.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)
Expand Down Expand Up @@ -394,7 +393,7 @@ func TestPeerRouting(t *testing.T) {
"field10": float64(10),
"long": "this is a test of the emergency broadcast system",
"meta.refinery.original_sample_rate": uint(2),
"meta.refinery.incoming_user_agent": getLibhoneyUserAgent(),
"meta.refinery.incoming_user_agent": "Test-Client",
"foo": "bar",
},
Metadata: map[string]any{
Expand Down Expand Up @@ -520,7 +519,7 @@ func TestEventsEndpoint(t *testing.T) {
"trace.trace_id": "1",
"foo": "bar",
"meta.refinery.original_sample_rate": uint(10),
"meta.refinery.incoming_user_agent": getLibhoneyUserAgent(),
"meta.refinery.incoming_user_agent": "Test-Client",
},
Metadata: map[string]any{
"api_host": "http://api.honeycomb.io",
Expand Down Expand Up @@ -570,7 +569,7 @@ func TestEventsEndpoint(t *testing.T) {
"trace.trace_id": "1",
"foo": "bar",
"meta.refinery.original_sample_rate": uint(10),
"meta.refinery.incoming_user_agent": getLibhoneyUserAgent(),
"meta.refinery.incoming_user_agent": "Test-Client",
},
Metadata: map[string]any{
"api_host": "http://api.honeycomb.io",
Expand Down Expand Up @@ -644,7 +643,7 @@ func TestEventsEndpointWithNonLegacyKey(t *testing.T) {
"trace.trace_id": traceID,
"foo": "bar",
"meta.refinery.original_sample_rate": uint(10),
"meta.refinery.incoming_user_agent": getLibhoneyUserAgent(),
"meta.refinery.incoming_user_agent": "Test-Client",
},
Metadata: map[string]any{
"api_host": "http://api.honeycomb.io",
Expand Down Expand Up @@ -694,7 +693,7 @@ func TestEventsEndpointWithNonLegacyKey(t *testing.T) {
"trace.trace_id": traceID,
"foo": "bar",
"meta.refinery.original_sample_rate": uint(10),
"meta.refinery.incoming_user_agent": getLibhoneyUserAgent(),
"meta.refinery.incoming_user_agent": "Test-Client",
},
Metadata: map[string]any{
"api_host": "http://api.honeycomb.io",
Expand Down Expand Up @@ -915,11 +914,3 @@ func BenchmarkDistributedTraces(b *testing.B) {
sender.waitForCount(b, b.N)
})
}

// ideally we should get this from libhoney, but we don't have a way to get it yet
// this can be removed if libhoney does provide it
func getLibhoneyUserAgent() string {
baseUserAgent := fmt.Sprintf("libhoney-go/%s", version.Version)
runtimeInfo := fmt.Sprintf("%s (%s/%s)", strings.Replace(runtime.Version(), "go", "go/", 1), runtime.GOOS, runtime.GOARCH)
return fmt.Sprintf("%s %s", baseUserAgent, runtimeInfo)
}
2 changes: 1 addition & 1 deletion route/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ func getUserAgentFromRequest(req *http.Request) string {
}

func addIncomingUserAgent(ev *types.Event, userAgent string) {
if userAgent != "" {
if userAgent != "" && ev.Data["meta.refinery.incoming_user_agent"] == nil {
ev.Data["meta.refinery.incoming_user_agent"] = userAgent
}
}
23 changes: 23 additions & 0 deletions route/route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/honeycombio/refinery/metrics"
"github.com/honeycombio/refinery/sharder"
"github.com/honeycombio/refinery/transmit"
"github.com/honeycombio/refinery/types"
"github.com/jonboulle/clockwork"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -721,3 +722,25 @@ func TestGetDatasetFromRequest(t *testing.T) {
})
}
}

func TestAddIncomingUserAgent(t *testing.T) {
t.Run("no incoming user agent", func(t *testing.T) {
event := &types.Event{
Data: map[string]interface{}{},
}

addIncomingUserAgent(event, "test-agent")
require.Equal(t, "test-agent", event.Data["meta.refinery.incoming_user_agent"])
})

t.Run("existing incoming user agent", func(t *testing.T) {
event := &types.Event{
Data: map[string]interface{}{
"meta.refinery.incoming_user_agent": "test-agent",
},
}

addIncomingUserAgent(event, "another-test-agent")
require.Equal(t, "test-agent", event.Data["meta.refinery.incoming_user_agent"])
})
}

0 comments on commit 869da1a

Please sign in to comment.