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

feat: add service target fields support to http and grpc requests #1287

Merged
merged 19 commits into from
Sep 27, 2022
Merged

feat: add service target fields support to http and grpc requests #1287

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
21bb1e5
feat: add service target fields support to http and grpc requests
kruskall Aug 6, 2022
e0928c4
Merge branch 'main' into feat/service-target-http
kruskall Aug 10, 2022
25194f0
fix: update service target name to include default port for some prot…
kruskall Aug 10, 2022
dc3b973
test: make sure 'SetHTTPRequest' is setting the correct service targe…
kruskall Aug 10, 2022
39b7b00
Merge branch 'main' into feat/service-target-http
kruskall Aug 10, 2022
c644584
fix: remove explicit http type now that the fallback mechanism is merged
kruskall Sep 14, 2022
b20d91c
Merge branch 'main' into feat/service-target-http
kruskall Sep 14, 2022
df30354
fix: readd explicit http service target type
kruskall Sep 14, 2022
1a40227
fix: remove explicit type again
kruskall Sep 15, 2022
6f0b1e4
test: mark spans as exit spans and update type on http test
kruskall Sep 15, 2022
b71201f
Merge branch 'main' into feat/service-target-http
kruskall Sep 15, 2022
bea474e
refactor: omit service target type for grpc
kruskall Sep 15, 2022
8bddb28
test: disable service target field assertion for grpc test
kruskall Sep 15, 2022
77a8519
Revert "test: disable service target field assertion for grpc test"
kruskall Sep 15, 2022
eafcc60
fix: omit service target fields on non-exit spans
kruskall Sep 19, 2022
bc60085
test: update tests for exit-span changes
kruskall Sep 19, 2022
3637393
Merge branch 'main' into feat/service-target-http
kruskall Sep 23, 2022
1fe6fc5
Revert "test: update tests for exit-span changes"
kruskall Sep 23, 2022
f5b75a5
test: remove service target fields assertions from non exit spans
kruskall Sep 23, 2022
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
4 changes: 4 additions & 0 deletions module/apmgrpc/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ func NewUnaryClientInterceptor(o ...ClientOption) grpc.UnaryClientInterceptor {
Resource: url.Host,
})
}
span.Context.SetServiceTarget(apm.ServiceTargetSpanContext{
Type: "grpc",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Type: "grpc",

Copy link
Member Author

Choose a reason for hiding this comment

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

Same issue, test is failing because it's not an exit span. Should we update this in the issue to omitservice.target for non-exit spans?

Copy link
Member

Choose a reason for hiding this comment

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

Same issue, test is failing because it's not an exit span. Should we update this in the issue to omitservice.target for non-exit spans?

Ah, and we don't have a way to mark some gRPC spans as exit spans yet...

Yes, let's just update this PR to omit for non-exit spans.

Copy link
Member Author

Choose a reason for hiding this comment

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

The span context doesn't know if the parent span is an exit span. I'm not sure how to proceed, should we add a field to the span context ? I'd prefer to avoid that as it feels like coupling different components

Copy link
Member

Choose a reason for hiding this comment

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

should we add a field to the span context ? I'd prefer to avoid that as it feels like coupling different components

I also don't think we add the exit flag as a field. Here's a couple of options that come to mind:

  1. Update Span.End to do something like this:
if s.exit {
        // The span was created as an exit span, but the user did not
        // manually set the service.target fields.
        s.setExitSpanServiceTarget()
} else {
        s.clearExitSpanServiceTarget()
}

Maybe like this:

func (s *Span) End() {
    ...
    s.setExitSpanServiceTarget()
    ...
}

func (s *Span) setExitSpanServiceTarget() {
    if !s.exit {
        // Clear any manually set `service.target.*` context.
        s.Context.SetServiceTarget(ServiceTargetSpanContext{})
        return
    }

    fallbackType := s.Subtype 
    if fallbackType == "" {
        fallbackType = s.Type
    }

    // Service target fields explicitly provided.
    if s.Context.setServiceTargetCalled {
    ...
}

func (sc *SpanContext) SetServiceTarget(target ServiceTargetSpanContext) {
    c.setServiceTargetCalled = true
    if service.Name == "" {
        s.model.Service = nil
        return
    }
    c.serviceTarget.Type = truncateString(service.Type)
    c.serviceTarget.Name = truncateString(service.Name)
    c.service.Target = &c.serviceTarget
    c.model.Service = &c.service
}
  1. We could pass span.exit into SpanContext.build() in buildModelSpan:

apm-agent-go/modelwriter.go

Line 156 in 99546a3

out.Context = sd.Context.build()

And build() would clear the model.SpanContext.Service field for non-exit spans.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for help! 🙇

Quick question before pushing more changes: do we have a way to mark grpc/http requests as exit spans ? The latest change is going to reduce coverage of this feature because the spans used in testing are not exit spans.

Copy link
Member

Choose a reason for hiding this comment

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

Quick question before pushing more changes: do we have a way to mark grpc/http requests as exit spans ? The latest change is going to reduce coverage of this feature because the spans used in testing are not exit spans.

Not at the moment. For gRPC, we always assume that the target service is instrumented and pass trace context. Thus it should not be considered an "exit" span. I think we should make this configurable. I opened an issue about doing this for apmhttp spans recently, and have just updated it to also take apmgrpc client spans into account: #1304

Copy link
Member

Choose a reason for hiding this comment

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

@kruskall sorry, I led you astray due to my own confusion around exit spans and trace-context propagation. We should be marking HTTP, gRPC, Elasticsearch, and some other spans as "exit spans" by default. See #1315

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries 😄

I think this PR grew in scope quite a lot, I've spli the fix in a separate PR: #1317

Name: url.Host,
})
}
return err
}
Expand Down
6 changes: 6 additions & 0 deletions module/apmgrpc/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ func testClientSpan(t *testing.T, traceparentHeaders ...string) {
Resource: tcpAddr.String(),
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "grpc",
Name: tcpAddr.String(),
},
},
}, clientSpans[0].Context)

serverTracer.Flush(nil)
Expand Down
6 changes: 6 additions & 0 deletions module/apmhttp/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ func TestClient(t *testing.T) {
Resource: serverAddr.String(),
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "http",
Name: serverAddr.String(),
},
},
HTTP: &model.HTTPSpanContext{
// Note no user info included in server.URL.
URL: serverURL,
Expand Down
12 changes: 12 additions & 0 deletions module/apmot/tracer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ func TestHTTPSpan(t *testing.T) {
Resource: "testing.invalid:8443",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "http",
Name: "testing.invalid:8443",
},
},
}, modelSpan.Context)
}

Expand Down Expand Up @@ -235,6 +241,12 @@ func TestHTTPSpanTraefik(t *testing.T) {
Resource: "202.1.1.1:5000",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "http",
Name: "202.1.1.1:5000",
},
},
Tags: model.IfaceMap{
{Key: "http_status_code", Value: "200"},
{Key: "router_name", Value: "api@internal"},
Expand Down
4 changes: 4 additions & 0 deletions spancontext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,10 @@ func (c *SpanContext) SetHTTPRequest(req *http.Request) {
Name: destinationServiceURL.String(),
Resource: destinationServiceResource,
})
c.SetServiceTarget(ServiceTargetSpanContext{
Type: "http",
Copy link
Member

Choose a reason for hiding this comment

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

According to https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans-service-target.md#field-values:

span.context.service.target.type should have the same value as span.subtype and fallback to span.type.

This would mean, for example, Elasticsearch spans would have service.target.type: elasticsearch (which is desirable). According to the pseudocode, we should only set Type if it is empty - and then, only to the span subtype or type.

What I'd suggest is we leave it empty here, and then set the default in Span.End()

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I was planning to do the fallback mechanism in a separate PR: #1294

This would mean, for example, Elasticsearch spans would have service.target.type: elasticsearch (which is desirable). According to the pseudocode, we should only set Type if it is empty - and then, only to the span subtype or type.

The current approach was mostly based on what destination.resource was doing. In particular, Elasticsearch would overwrite the service target fields (and the old destination resource) because it is calling SetServiceTarget after SetHTTPRequest.
Another option is to move the destination resource and service target fields outside of SetHTTPRequest and leave the responsibility of setting them to the caller.

What I'd suggest is we leave it empty here, and then set the default in Span.End()

I think this approach won't work: span.End will only set the service target fields (or destination resource) if those fields weren't explicitly set (see the PR linked above for the code, it's based on what destination resource was doing). We could add another toggle/field to differentiate whether span.End should overwrite the fields but I feel like it would make the logic less robust.

WDYT ?

kruskall marked this conversation as resolved.
Show resolved Hide resolved
Name: destinationServiceResource,
})
}

// SetHTTPStatusCode records the HTTP response status code.
Expand Down
4 changes: 4 additions & 0 deletions spancontext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ func TestSpanContextSetHTTPRequest(t *testing.T) {
Resource: tc.resource,
},
}, spans[0].Context.Destination)
assert.Equal(t, &model.ServiceTargetSpanContext{
Type: "http",
Name: tc.resource,
}, spans[0].Context.Service.Target)
})
}
}
Expand Down