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

[kruskall]

currently blocked by #1315

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-09-23T19:03:09.301+0000

• Duration: 58 min 57 sec

Test stats 🧪

Test	Results
Failed	0
Passed	8554
Skipped	201
Total	8755

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark test.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[apmmachine]

🌐 Coverage report

Name	Metrics % (covered/total)	Diff
Packages	100.0% (59/59)	💚
Files	99.346% (152/153)	👍
Classes	96.275% (336/349)	👍
Methods	90.405% (961/1063)	👍 0.094
Lines	82.213% (11199/13622)	👎 -0.022
Conditionals	100.0% (0/0)	💚

[axw]

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()

[kruskall]

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 ?

[axw]

LGTM, thanks. Let's do the same thing for grpc. Can you please open an issue to omit service.target for non-exit spans?

[axw]

Suggested change

Type: "grpc",

[kruskall]

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?

[axw]

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.

[kruskall]

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

[axw]

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
}

2. 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.

[kruskall]

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.

[axw]

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

[axw]

@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

[kruskall]

No worries 😄

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

[axw]

🎉 🎉 🎉

LGTM, thanks for working through all the interconnected issues :)