Backend granularity (service.target.*) clarifications #674

trentm · 2022-08-17T17:18:59Z

I have some questions and clarifications on the service.target.* backend granularity changes. I'll try to ask them in comments and code-review-comments below.

Update 2022-09-13: There was a lot of earlier discussion of Q1 - Q7 on this PR. If you are a new reviewer of this PR, you can probably skip all that earlier discussion and just look at the current file changes. I added some "REVIEW NOTE:" review comments that add context.

checklist

apmmachine · 2022-08-17T17:22:31Z

💚 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-30T05:17:00.935+0000
Duration: 3 min 16 sec

specs/agents/tracing-spans-service-target.md

specs/agents/tracing-spans-destination.md

trentm · 2022-08-17T17:59:34Z

Q4: The Java agent has setServiceTarget(...) on Span, but also on Transaction. Why on Transaction? What is the case where a Transaction can be an "exitSpan" where we'd want to support having transaction.context.service.target.*? A guess is perhaps this is with the OTel Bridge where an OTel Span without a current parent transaction is converted to a Transaction.

trentm · 2022-08-17T18:01:06Z

Q5: tracing-spans-service-target.md has an OTel Bridge section that includes:

APM server already infers the span.destination.service.resource value from OTel span attributes, this algorithm needs to be updated in order to also infer the values of span.context.service.target.* fields.

However, the pseudo-code in "tracing-api-otel.md" still shows setting destination.service.resource. Should this spec pseudo-code be updated to reflect what was done in the Java agent implementation here: https://github.com/elastic/apm-agent-java/pull/2578/files#diff-5d63ee730051382a036b760e5eb38a80eb6d370e93f00e4bc9d998f1338d7025 ?

trentm · 2022-08-17T23:31:42Z

Q6: https://github.com/elastic/apm/blob/main/specs/agents/handling-huge-traces/tracing-spans-compress.md#consecutive-same-kind-compression-strategy says:

When applying this compression strategy, the span.name is set to Calls to $span.destination.service.resource.

But the Java agent impl uses service.target.*: https://github.com/elastic/apm-agent-java/pull/2578/files#diff-0a29377db9d5c6270ff4865a5585c9cd7633f33eb7a88c57aaa821da9e8c2941R380-R402

Should I update the compressed-spans spec to use the alg in the Java impl?

trentm · 2022-08-18T18:06:20Z

Q7: Two questions on the S3 spec

Should context.db.instance be changed to be the $bucketNameIfAvailable rather than the current $region? Typically for DB instrumentations the context.service.target.name is the same as context.db.instance. However, in the current spec for S3 they do not match. I think it was an oversight of the original AWS S3 spec that context.db.instance was decided to be the region rather than the bucket name.
Should context.destination.service.resource be changed to include an 's3/' prefix?
Currently the S3 spec has service.target = { type: 's3', name: '$bucketNameIfAvailable' }. This means that following the usual inference of context.destination.service.resource from context.service.target we would expect "s3/$bucketName" (or "s3" if there is no bucket name, e.g. for the "ListBuckets" API call) rather than the currently specified "$bucketNameIfAvailable".

SylvainJuge · 2022-08-24T11:52:10Z

Q4: The Java agent has setServiceTarget(...) on Span, but also on Transaction. Why on Transaction? What is the case where a Transaction can be an "exitSpan" where we'd want to support having transaction.context.service.target.*? A guess is perhaps this is with the OTel Bridge where an OTel Span without a current parent transaction is converted to a Transaction.

This is only because for the user-facing API we have Transation class inherit from Span which is a design oversight and that we can't change easily without breaking binary compatibility.

SylvainJuge · 2022-08-24T12:11:27Z

Q5: tracing-spans-service-target.md has an OTel Bridge section that includes:

APM server already infers the span.destination.service.resource value from OTel span attributes, this algorithm needs to be updated in order to also infer the values of span.context.service.target.* fields.

However, the pseudo-code in "tracing-api-otel.md" still shows setting destination.service.resource. Should this spec pseudo-code be updated to reflect what was done in the Java agent implementation here: https://github.com/elastic/apm-agent-java/pull/2578/files#diff-5d63ee730051382a036b760e5eb38a80eb6d370e93f00e4bc9d998f1338d7025 ?

Yes, this pseudo-code should be updated to also infer the new service.target.* fields.

SylvainJuge · 2022-08-24T12:12:37Z

Q6: https://github.com/elastic/apm/blob/main/specs/agents/handling-huge-traces/tracing-spans-compress.md#consecutive-same-kind-compression-strategy says:

When applying this compression strategy, the span.name is set to Calls to $span.destination.service.resource.

But the Java agent impl uses service.target.*: https://github.com/elastic/apm-agent-java/pull/2578/files#diff-0a29377db9d5c6270ff4865a5585c9cd7633f33eb7a88c57aaa821da9e8c2941R380-R402

Should I update the compressed-spans spec to use the alg in the Java impl?

Yes

SylvainJuge · 2022-08-24T12:30:31Z

Q7: Two questions on the S3 spec

Should context.db.instance be changed to be the $bucketNameIfAvailable rather than the current $region? Typically for DB instrumentations the context.service.target.name is the same as context.db.instance. However, in the current spec for S3 they do not match. I think it was an oversight of the original AWS S3 spec that context.db.instance was decided to be the region rather than the bucket name.

Should context.destination.service.resource be changed to include an 's3/' prefix?
Currently the S3 spec has service.target = { type: 's3', name: '$bucketNameIfAvailable' }. This means that following the usual inference of context.destination.service.resource from context.service.target we would expect "s3/$bucketName" (or "s3" if there is no bucket name, e.g. for the "ListBuckets" API call) rather than the currently specified "$bucketNameIfAvailable".

For S3, I think that we need to discuss what is the best option here, here are a few thoughts on this

for me db.instance should only be used for databases ideally, abusing that field for other purposes creates extra complexity afterwards.
using db.instance with the bucket name, even if S3 is not technically a database that would at least be consistent with what we have done for relational databases.
the cloud fields in ECS do not provide a normalized way to store an S3 bucket name, so maybe for now the db.instance might be the best compromise for now.
using the region in db.instance duplicates the cloud.target.region that was added quite recently in ECS, and using it to provide better granularity should only be done when there is no better option, for example with Azure tables that do not have a database and the best thing we can have is the account (because access to the region isn't easy).

Update: I found that there has been a stage 0 RFC to add a field in ECS that would allow to store the the bucket name, but it's far from being done.

SylvainJuge · 2022-09-09T13:11:07Z

Paraphrasing myself from my last conversation with @trentm (before I forget about it):

On the agent side, there are two places where the new and old fields can be set:

explicitly at instrumentation level, which might be a bit tedious but allows to handle special values (for example if we want to keep the s3 oddity as-is) (1)
when the span completes, in which case we rely only on the captured fields (2)

For (2), we can infer resource from the new service.target fields for the general case, but can't deal with special cases like s3.
However, when the values of both old and new fields have been set in (1), we do not need to infer anything and we only need to use the provided values (which will also be the case when the user changes those values).

So, while modifying the convention for S3 would be great, it is not strictly required if we handle it as a "special case" explicitly in all S3 instrumentations.

…rver' is deprecated in favour of 'mssql'

trentm · 2022-09-09T21:07:15Z

Q6: ...

Done in bebe220

…ice.target The, perhaps surprising, use of `span.type === "external"` is discussed in more detail here: https://github.com/elastic/apm-agent-nodejs/blob/a10bccfe797577f8414c957aaf1ec50ea581e8b9/lib/instrumentation/span.js#L146-L173 This is for my Q3.

…span service.target rather than destination.service.resource

trentm · 2022-09-09T22:58:30Z

Q5

The OTel Bridge compatibility mapping code is updated to set service.target (rather than destination.service.resource) in commit 2a66234.

trentm · 2022-09-09T23:04:08Z

@SylvainJuge I believe all my questions have been answered, except the S3 discussion (Q7). I've spent a bit more time with that and have a proposal that I'll run by you and then create a separate spec PR for that.

I'm ready for a regular re-review of this spec PR from you, when you have a chance.

trentm

Add notes for reviewers.

trentm · 2022-09-09T23:06:32Z

specs/agents/handling-huge-traces/tracing-spans-compress.md

+    }
+}
+```
+


REVIEW NOTE: This change updates the composite span.name calculation to match the implementation in the Java Agent here: https://github.com/elastic/apm-agent-java/blob/32611fe5174cf7c67f12c885cb6759176749f243/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java#L381-L403

trentm · 2022-09-09T23:08:57Z

specs/agents/tracing-api-otel.md

@@ -169,13 +169,14 @@ if (span_kind == 'SERVER' && (isRpc || isHttp)) {
 }
 ```

-#### Span type, sub-type and destination service resource
+#### Span type, sub-type and service target


REVIEW NOTE: This section updates the OTel Bridge compatibility mapping logic to set span.context.service.target.* rather than span.context.destination.service.resource. (Inferring the destination.service.resource value is handled by the general logic for all spans in tracing-spans-destination.md below.) It should correspond to the logic in the Java agent here: https://github.com/elastic/apm-agent-java/blob/32611fe5174cf7c67f12c885cb6759176749f243/apm-agent-plugins/apm-opentelemetry/apm-opentelemetry-plugin/src/main/java/co/elastic/apm/agent/opentelemetry/sdk/OTelSpan.java#L148-L250

trentm · 2022-09-09T23:12:22Z

specs/agents/tracing-instrumentation-db.md

@@ -227,7 +227,7 @@ The Elasticsearch cluster name is not always available in ES clients, as a resul
 | MySQL                | `mysql`     |
 | MariaDB              | `mariadb`   |
 | PostgreSQL           | `postgresql`|
-| Microsoft SQL server | `sqlserver` |
+| Microsoft SQL server | `mssql`     |


REVIEW NOTE: I think this use of sqlserver was unintended. "json-specs/span_types.json" suggests that "sqlserver" is deprecated in favour of "mssql".

trentm · 2022-09-09T23:19:08Z

specs/agents/tracing-spans-destination.md

 else
-  subtype ?: type
+  "${span.context.service.target.type}/${span.context.service.target.name}"


REVIEW NOTE: This section updates the logic for inferring context.destination.service.resource from context.service.target.* (and span.type). This is the logic I'm using in the Node.js APM agent here: https://github.com/elastic/apm-agent-nodejs/blob/a10bccfe797577f8414c957aaf1ec50ea581e8b9/lib/instrumentation/span.js#L146-L173

That link provides some more discussion on why else if (span.type == 'external') was used. My observation was that the exceptional case in the spec text and "otel_bridge.feature" -- where we skip the "${service.target.type}/" prefix -- is for HTTP and RPC system spans (like gRPC and Apache Dubbo). These are exactly the set of spans that we specify should use span.type == 'external'. It seems to me a much clearer check that using other attributes such as:

} else if (!context.db && !context.message && context.http && context.http.url) {

which is closer to the old logic that was used for inferring destination.service.resource. Thoughts?

I agree, it's simpler to rely on span.type = "external" here, that would probably be a very good argument to enforce adherence to the spec for span types & subtypes.

trentm · 2022-09-09T23:23:16Z

specs/agents/tracing-spans-service-target.md

@@ -105,35 +105,37 @@ This specification assumes that values for `span.type` and `span.subtype` fit th
 - `span.context.service.target.name` depends on the span context attributes

 On agents, the following algorithm should be used to infer the values for `span.context.service.target.*` fields.
+


REVIEW NOTE: This section slightly tweaks the pseudo-code to infer context.service.target.* from other span fields to avoid a couple gotchas that I hit:

The if (!service_target.type) check in JavaScript would accidentally override service_target.type if it had explicitly been set to the empty string.

The if (span.context.db.instance) branch was changed to if (span.context.db) to use the DB logic for DB spans even if the span didn't happen to have a "db.instance" set -- which is less surprising and matches the old pseudo-code for inferring destination.service.resource.

SylvainJuge

LGTM, thanks a lot for helping clarify this !

SylvainJuge · 2022-09-13T08:57:15Z

specs/agents/tracing-spans-destination.md

 else
-  subtype ?: type
+  "${span.context.service.target.type}/${span.context.service.target.name}"


I agree, it's simpler to rely on span.type = "external" here, that would probably be a very good argument to enforce adherence to the spec for span types & subtypes.

Backend granularity (service.target.*) clarifications

d42311c

trentm self-assigned this Aug 17, 2022

tweak, nest-if-guards for db/message cases

dc82a08

exists-check for 'name' case as well

ea9c4c7

trentm commented Aug 17, 2022

View reviewed changes

specs/agents/tracing-spans-service-target.md Outdated Show resolved Hide resolved

specs/agents/tracing-spans-service-target.md Outdated Show resolved Hide resolved

trentm added 3 commits August 17, 2022 10:46

add the start of my Q3

5f1197d

Merge branch 'main' into trentm/backend-granularity-q

fc95ebd

first mismerge

5df58c6

trentm commented Aug 17, 2022

View reviewed changes

specs/agents/tracing-spans-destination.md Outdated Show resolved Hide resolved

trentm requested a review from SylvainJuge August 17, 2022 18:01

trentm mentioned this pull request Aug 17, 2022

feat: service.target.* to improve backend granularity elastic/apm-agent-nodejs#2882

Merged

trentm added 3 commits September 9, 2022 14:03

update composite span.name to be calculated from service.target.*

bebe220

tweak this description

de440a0

I believe this was a typo, json-specs/span_types.json suggests 'sqlse…

d315524

…rver' is deprecated in favour of 'mssql'

trentm added 3 commits September 9, 2022 15:17

this was my Q2, but I accidentally inverted the logic

7992292

(for Q5) update OTel Bridge compatibility mapping pseudo-code to set …

2a66234

…span service.target rather than destination.service.resource

trentm commented Sep 9, 2022

View reviewed changes

SylvainJuge approved these changes Sep 13, 2022

View reviewed changes

trentm marked this pull request as ready for review September 13, 2022 16:07

trentm requested review from a team as code owners September 13, 2022 16:07

trentm removed the request for review from a team September 13, 2022 16:07

trentm mentioned this pull request Sep 13, 2022

Remove context.db from S3 spans: S3 isn't a database #683

Merged

11 tasks

basepi approved these changes Sep 19, 2022

View reviewed changes

astorm approved these changes Sep 20, 2022

View reviewed changes

trentm merged commit 4a5e72b into main Oct 3, 2022

trentm deleted the trentm/backend-granularity-q branch October 3, 2022 19:32

trentm mentioned this pull request Oct 3, 2022

Backend granularity (service.target.*) clarifications #697

Open

4 tasks

trentm mentioned this pull request Oct 17, 2022

S3 instrumentation does not contain s3 in destination.service.resource name elastic/apm-agent-java#2849

Closed

kruskall mentioned this pull request Oct 19, 2022

fix: update compressed span name logic for spec change elastic/apm-agent-go#1336

Merged

JonasKunz mentioned this pull request Dec 16, 2022

Added s3-prefix to S3 destination.service.resource #738

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend granularity (service.target.*) clarifications #674

Backend granularity (service.target.*) clarifications #674

trentm commented Aug 17, 2022 •

edited

Loading

apmmachine commented Aug 17, 2022 •

edited by jenkins-apm-ci bot

Loading

Build stats

trentm commented Aug 17, 2022

trentm commented Aug 17, 2022

trentm commented Aug 17, 2022

trentm commented Aug 18, 2022

SylvainJuge commented Aug 24, 2022

SylvainJuge commented Aug 24, 2022

SylvainJuge commented Aug 24, 2022

SylvainJuge commented Aug 24, 2022 •

edited

Loading

SylvainJuge commented Sep 9, 2022

trentm commented Sep 9, 2022

trentm commented Sep 9, 2022

trentm commented Sep 9, 2022

trentm left a comment

trentm Sep 9, 2022

trentm Sep 9, 2022 •

edited

Loading

trentm Sep 9, 2022

trentm Sep 9, 2022

SylvainJuge Sep 13, 2022

trentm Sep 9, 2022

SylvainJuge left a comment

SylvainJuge Sep 13, 2022

		@@ -105,35 +105,37 @@ This specification assumes that values for `span.type` and `span.subtype` fit th
		- `span.context.service.target.name` depends on the span context attributes

		On agents, the following algorithm should be used to infer the values for `span.context.service.target.*` fields.

Backend granularity (service.target.*) clarifications #674

Backend granularity (service.target.*) clarifications #674

Conversation

trentm commented Aug 17, 2022 • edited Loading

checklist

apmmachine commented Aug 17, 2022 • edited by jenkins-apm-ci bot Loading

💚 Build Succeeded

Build stats

trentm commented Aug 17, 2022

trentm commented Aug 17, 2022

trentm commented Aug 17, 2022

trentm commented Aug 18, 2022

SylvainJuge commented Aug 24, 2022

SylvainJuge commented Aug 24, 2022

SylvainJuge commented Aug 24, 2022

SylvainJuge commented Aug 24, 2022 • edited Loading

SylvainJuge commented Sep 9, 2022

trentm commented Sep 9, 2022

trentm commented Sep 9, 2022

trentm commented Sep 9, 2022

trentm left a comment

Choose a reason for hiding this comment

trentm Sep 9, 2022

Choose a reason for hiding this comment

trentm Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

trentm Sep 9, 2022

Choose a reason for hiding this comment

trentm Sep 9, 2022

Choose a reason for hiding this comment

SylvainJuge Sep 13, 2022

Choose a reason for hiding this comment

trentm Sep 9, 2022

Choose a reason for hiding this comment

SylvainJuge left a comment

Choose a reason for hiding this comment

SylvainJuge Sep 13, 2022

Choose a reason for hiding this comment

trentm commented Aug 17, 2022 •

edited

Loading

apmmachine commented Aug 17, 2022 •

edited by jenkins-apm-ci bot

Loading

SylvainJuge commented Aug 24, 2022 •

edited

Loading

trentm Sep 9, 2022 •

edited

Loading