feat: add fallback service target type for exit spans #1294

kruskall · 2022-08-11T18:29:05Z

This is the final step for complete service target fields
support. Exit spans will fallback to type and subtype similar
to what the deprecated destination resource was doing.

Assertions have been added to tests to verify the service target
type is not overwritten if previously set and that the fallback
mechanism is working properly.

related to #1276

Closes #1294

This is the final step for complete service target fields support. Exit spans will fallback to type and subtype similar to what the deprecated destination resource was doing. Assertions have been added to tests to verify the service target type is not overwritten if previously set and that the fallback mechanism is working properly.

apmmachine · 2022-08-11T19:21:14Z

💚 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-14T02:13:01.617+0000
Duration: 53 min 40 sec

Test stats 🧪

Test	Results
Failed	0
Passed	8531
Skipped	201
Total	8732

🤖 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 · 2022-08-11T19:21:23Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`59/59`)	💚
Files	99.346% (`152/153`)	👍
Classes	96.275% (`336/349`)	👍
Methods	90.226% (`960/1064`)	👍 0.009
Lines	82.148% (`11159/13584`)	👎 -0.001
Conditionals	100.0% (`0/0`)	💚

axw · 2022-08-12T02:29:24Z

span.go

+	if s.exit && !s.Context.setServiceTargetCalled {
+		// The span was created as an exit span, but the user did not
+		// manually set the service.target fields.
+		s.setExitSpanServiceTarget()
+	}


This doesn't quite follow the logic described at https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans-service-target.md#field-values. I think users should be able to set just the Name, and have the Type be inferred from the span type/subtype. (I realise this is different to what we had with service destination, but let's just ignore that since it's being replaced.)

Rather than checking if the user has called the method, how about something like this:

if s.exit { s.Context.setDefaultServiceTarget(s.Type, s.Subtype) } ... func (c *SpanContext) setServiceTargetDefaults(spanType, spanSubtype string) { if c.serviceTarget.Type == "" { c.serviceTarget = spanSubtype if c.serviceTarget.Type == "" { c.serviceTarget = spanType } } if c.serviceTarget.Name == "" { // ... } c.SetServiceTarget(ServiceTargetSpanContext{Type: c.serviceTarget.Type, Name: c.serviceTarget.Name}) }

Ok, just a question to clarify: it seems that would infer the value even if an explicit empty name/type was set. Is that intended ?

I thought so, since the type can always be inferred and the name is optional, but the spec also says this:

Agents SHOULD provide an API entrypoint to set the value of span.context.service.target.type and span.context.service.target.name, setting an empty or null value on both of those fields allows the user to discard the inferred values.

@SylvainJuge why would a user need to disable the inference of service.target.type or service.target.name?

Hi !

I remember it was requested at some point for the Java agent in order to provide a work-around when our heuristics fail and or the user wants more control on how things are named (and maybe ignore a few dependencies):

allow to split a dependency when the heuristic groups everything (for example when calls are made to a gateway, we currently only show the gateway, the user might want to manually split if the app has some knowledge about this).

allow to limit cardinality : for example a single database server with dozens of databases that might be better grouped together, or a web-crawling app that has one dependency per DNS entry by default.

@SylvainJuge let me know if I understand correctly: what you're saying is that in some cases the inferred type & name may not be ideal, so users may wish to override the values and provide either more specific values (e.g. in the case of a gateway) or less specific values (e.g. in the case of many databases instances).

That's a good argument for allowing users to manually set non-empty values, but I don't see why we should completely omit the service.target.* fields if a user sets them both to null/empty values.

@SylvainJuge let me know if I understand correctly: what you're saying is that in some cases the inferred type & name may not be ideal, so users may wish to override the values and provide either more specific values (e.g. in the case of a gateway) or less specific values (e.g. in the case of many databases instances).

Yes, that's exactly what I meant.

That's a good argument for allowing users to manually set non-empty values, but I don't see why we should completely omit the service.target.* fields if a user sets them both to null/empty values.

Omitting the fields when empty would be consistent with what we do for resource field (spec) for context.destination.service.resource :

Agents SHOULD offer a public API to set this field so that users can customize the value if the generic mapping is not
sufficient. If set to null or an empty value, agents MUST omit the span.destination.service field altogether, thus
providing a way to manually disable the automatic setting/inference of this field (e.g. in order to remove a node
from a service map or an external service from the dependencies table).

Also, when set to empty/null values, omitting both destination.service.resource and service.target.* would avoid making the apm-server try to infer any of those fields which could lead to inconsitencies.

Thanks @SylvainJuge.

How about this then:

if the user does not call SetServiceTarget at all, we'll use inferred values for both (where possible)

if the user calls SetServiceTarget with an empty name, this will disable inference

if the user calls SetServiceTarget with a non-empty name, but empty type, we'll use the specified name and infer the type

I think that covers the intended use case of controlling the service.target.name cardinality.

How about this then:

* if the user does not call SetServiceTarget at all, we'll use inferred values for both (where possible) * if the user calls SetServiceTarget with an empty name, this will disable inference * if the user calls SetServiceTarget with a non-empty name, but empty type, we'll use the specified name and infer the type

I think that covers the intended use case of controlling the service.target.name cardinality.

I've updated the code to reflect this!

axw · 2022-09-08T01:49:48Z

spancontext.go

@@ -273,6 +277,7 @@ func (c *SpanContext) SetDestinationService(service DestinationServiceSpanContex

 // SetServiceTarget sets the service target info in the context.
 func (c *SpanContext) SetServiceTarget(service ServiceTargetSpanContext) {
+	c.setServiceTargetCalled = true


To implement this part of the spec:

Agents SHOULD provide an API entrypoint to set the value of span.context.service.target.type and span.context.service.target.name, setting an empty or null value on both of those fields allows the user to discard the inferred values.

Should we check here if the specified name is empty, and then just set c.setServiceTargetCalled = true, set c.service.Target = nil, and return?

i.e.

// SetServiceTarget sets the service target info in the context. // // If service.Name is empty, then any previously set service target values // will be cleared, and inference of service target values will be disabled. func (c *SpanContext) SetServiceTarget(service ServiceTargetSpanContext) { c.setServiceTargetCalled = true if service.Name == "" { c.service.Target = nil } else { c.serviceTarget.Type = truncateString(service.Type) c.serviceTarget.Name = truncateString(service.Name) c.service.Target = nil c.model.Service = &c.service } }

axw · 2022-09-08T02:01:38Z

span.go

+	if s.exit && !s.Context.setServiceTargetCalled {
+		// The span was created as an exit span, but the user did not
+		// manually set the service.target fields.
+		s.setExitSpanServiceTarget()
+	}


Thanks @SylvainJuge.

How about this then:

if the user does not call SetServiceTarget at all, we'll use inferred values for both (where possible)

if the user calls SetServiceTarget with an empty name, this will disable inference

if the user calls SetServiceTarget with a non-empty name, but empty type, we'll use the specified name and infer the type

I think that covers the intended use case of controlling the service.target.name cardinality.

if the user does not call SetServiceTarget at all, we'll use inferred values for both (where possible) if the user calls SetServiceTarget with an empty name, this will disable inference if the user calls SetServiceTarget with a non-empty name, but empty type, we'll use the specified name and infer the type

axw

LGTM :) Just a couple of minor things

span_test.go

span.go

Co-authored-by: Andrew Wilkins <[email protected]>

github-actions bot added the agent-go label Aug 11, 2022

kruskall mentioned this pull request Aug 11, 2022

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

Merged

axw reviewed Aug 12, 2022

View reviewed changes

axw reviewed Sep 8, 2022

View reviewed changes

simitt added the state: blocked label Sep 13, 2022

kruskall requested a review from axw September 13, 2022 16:43

axw mentioned this pull request Sep 14, 2022

[META 659] Backend dependencies granularity for NoSQL and Messaging #1267

Closed

Merge branch 'main' into fallback/service-target-type

6aadf35

axw approved these changes Sep 14, 2022

View reviewed changes

span_test.go Outdated Show resolved Hide resolved

span.go Show resolved Hide resolved

axw removed the state: blocked label Sep 14, 2022

kruskall and others added 2 commits September 14, 2022 04:02

docs: update test comment to reference the correct field

5b1b329

Co-authored-by: Andrew Wilkins <[email protected]>

test: add service target infer testcase with explicit name

539c8ea

kruskall enabled auto-merge (squash) September 14, 2022 02:13

kruskall merged commit 1645e82 into elastic:main Sep 14, 2022

kruskall deleted the fallback/service-target-type branch October 19, 2022 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add fallback service target type for exit spans #1294

feat: add fallback service target type for exit spans #1294

kruskall commented Aug 11, 2022 •

edited by axw

Loading

apmmachine commented Aug 11, 2022 •

edited

Loading

Build stats

Test stats 🧪

apmmachine commented Aug 11, 2022 •

edited

Loading

axw Aug 12, 2022

kruskall Aug 12, 2022

axw Aug 12, 2022

SylvainJuge Aug 22, 2022

axw Aug 24, 2022

SylvainJuge Aug 24, 2022

axw Sep 8, 2022

kruskall Sep 13, 2022

axw Sep 8, 2022

axw Sep 8, 2022

axw left a comment

feat: add fallback service target type for exit spans #1294

feat: add fallback service target type for exit spans #1294

Conversation

kruskall commented Aug 11, 2022 • edited by axw Loading

apmmachine commented Aug 11, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

apmmachine commented Aug 11, 2022 • edited Loading

🌐 Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

kruskall commented Aug 11, 2022 •

edited by axw

Loading

apmmachine commented Aug 11, 2022 •

edited

Loading

apmmachine commented Aug 11, 2022 •

edited

Loading