Add unit test for OTLP receiver in jaeger-v1 #6541
Conversation
Signed-off-by: chahatsagarmain <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6541 +/- ##
==========================================
+ Coverage 95.93% 95.94% +0.01%
==========================================
Files 365 365
Lines 20616 20607 -9
==========================================
- Hits 19778 19772 -6
+ Misses 638 636 -2
+ Partials 200 199 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: chahatsagarmain <[email protected]>
|
dependent PR was merged |
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
yurishkuro
added
the
changelog:test
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
…into e2e-otlp Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
…into e2e-otlp Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
yurishkuro
changed the title
yurishkuro
added
changelog:test
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
…into e2e-otlp Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahat sagar <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
| var receivedTraces atomic.Pointer[ptrace.Traces] | ||
| var receivedCtx atomic.Pointer[context.Context] |
There was a problem hiding this comment.
you cannot share state between tests like this, it creates side effects. HTTP and gRPC should be tested in complete isolation.
Signed-off-by: chahatsagarmain <[email protected]>
| ) | ||
| require.NoError(t, err) | ||
| ctx := context.Background() | ||
| defer rec.Shutdown(ctx) |
There was a problem hiding this comment.
from L852 you are repeating a lot of code for each test. You can create a helper function that will initialize the receiver (it can be a lambda function).
| receivedTraces.Store(&storeTrace) | ||
| receivedCtx.Store(&storeContext) | ||
| }).Return(nil) | ||
| // Can't send tenancy headers with http request to OTLP receiver |
There was a problem hiding this comment.
old comment
|
|
||
| func TestOTLPReceiverWithV2Storage(t *testing.T) { | ||
| // Send trace via HTTP | ||
| t.Run("Send trace data using HTTP connection", func(t *testing.T) { |
There was a problem hiding this comment.
there are multiple ways to organize the tests, yours makes the least sense to me.
Option 1: two top-level functions, with shared helper functions
Option 2: a single test function with table-driven tests, no setup helpers needed but each test has a lambda function parameter executeRequest implemented with two other functions, for http and grpc separately
Your test is only checking success path, does not check scenario when incoming tenant is invalid.
Signed-off-by: chahatsagarmain <[email protected]>
| tenants := httpMd.Metadata.Get(c.tenancyMgr.Header) | ||
|
|
||
| if len(tenants) > 0 { | ||
| if !c.tenancyMgr.Valid(tenants[0]) { |
There was a problem hiding this comment.
We don't want to duplicate validation. You are missing other checks like the one that causes return "", status.Errorf(codes.PermissionDenied, "missing tenant header")
This function needs to be constructed of two parts, one is extracting the tenant info, another is validation.
And I would prefer to consolidate all this in pkg/tenancy/getValidTenant (which can be made public). It's a kind of a mess there too. We have four possible sources of tenant:
- it may already be in the Context
- it may be in the HTTP request headers
- it may be in gRPC request headers, which are accessible via Context via
metadata - finally, it may be in the Context as OTEL's
client.Metadata
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahat sagar <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
pkg/tenancy/grpc.go
Outdated
| var tenant string | ||
| var err error | ||
| if tenant = GetTenant(ctx); tenant != "" { | ||
| if !tm.Valid(tenant) { |
There was a problem hiding this comment.
please refactor such that you don't repeat logic. You can have a separate helper to extract the tenants (with early returns), and here validate it once.
| if tenant != "" { | ||
| if !tc.Valid(tenant) { | ||
| return tenant, status.Errorf(codes.PermissionDenied, "unknown tenant") | ||
| func GetValidTenant(ctx context.Context, tm *Manager) (string, error) { |
There was a problem hiding this comment.
please add a unit test for full coverage. Currently
$ go test -cover ./pkg/tenancy/
ok github.com/jaegertracing/jaeger/pkg/tenancy 0.483s coverage: 100.0% of statements
Signed-off-by: chahatsagarmain <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test