fix: update compressed span name logic for spec change by kruskall · Pull Request #1336 · elastic/apm-agent-go

kruskall · 2022-10-19T15:30:50Z

The logic to infer the compressed span name has been updated to use the service.target.* fields.
Update the code to be compliant with the specification.

Closes #1331
See elastic/apm#697
See elastic/apm#674

Not all the changes to the spec apply to the go agent:

ot bridge is not fully implemented (See [META 524] OpenTelemetry Bridge #1140)
the logic to infer the service.target.* fields has been clarified but we are already compliant with the spec

The logic to infer the compressed span name has been updated to use the service.target.* fields. Update the code to be compliant with the specification.

ghost · 2022-10-19T16:28:04Z

💚 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-12-05T01:17:38.568+0000
Duration: 61 min 32 sec

Test stats 🧪

Test	Results
Failed	0
Passed	8767
Skipped	222
Total	8989

🤖 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!)

ghost · 2022-10-19T16:28:14Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`60/60`)	💚
Files	99.355% (`154/155`)	👍
Classes	96.317% (`340/353`)	👍
Methods	90.349% (`983/1088`)	👍 0.009
Lines	82.148% (`11440/13926`)	👎 -0.064
Conditionals	100.0% (`0/0`)	💚

axw

Code looks fine. Are we missing some tests?

kruskall · 2022-11-18T05:44:25Z

Code looks fine. Are we missing some tests?

Sorry for the late reply, I don't think we are. The composite span name should be covered by the current tests: see https://github.com/elastic/apm-agent-go/blob/main/span_test.go#L634 and https://github.com/elastic/apm-agent-go/blob/main/span_test.go#L754.

I've added a couple more assertions in the remaining compression span tests for consistency. Let me know what you think!

axw · 2022-11-18T07:23:59Z

@kruskall there are now 4 possible ways the composite span name can be constructed, whereas there was just 1 before. I think we should have tests that cover each of those to ensure we maintain compatibility with the spec. Do those tests you pointed out fully cover the spec? I don't see how they could have, seeing as they didn't break.

kruskall · 2022-12-04T16:05:30Z

Good point! I've added a new test to cover the spec.

axw · 2022-12-05T00:38:21Z

span_compressed.go

+		if s.Context.serviceTarget.Name == "" {
+			return compressedSpanSameKindName + "unknown"
+		} else {
+			return compressedSpanSameKindName + s.Context.serviceTarget.Name


IIANM it's impossible to reach this code path, since we're guaranteed to infer a service.target.type value if service.target.name is set. Is that correct? Perhaps the spec should be updated to remove the pseudo-code path?

This is correct. Should we merge this and open a followup PR once the spec is updated ?

We could also remove the line since it's never reached. Let me know what you think!

I think it's fine to leave it and follow up.

marclop · 2023-03-29T01:33:24Z

Verified with 8.7.0 BC9 using the latest main (332875e)

All target fields populated

package main

import (
	"fmt"
	"time"

	"go.elastic.co/apm/v2"
)

func main() {
	tracer := apm.DefaultTracer()
	tracer.SetSpanCompressionEnabled(true)
	tracer.SetSpanCompressionSameKindMaxDuration(time.Millisecond)
	tracer.SetExitSpanMinDuration(0)
	currentTime := time.Now()
	tx := tracer.StartTransactionOptions("8.7.0", "testplan", apm.TransactionOptions{
		Start: currentTime,
	})
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM users", "mysql", "db", "myhost"),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM catalog", "mysql", "db", "myhost"),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM orders", "mysql", "db", "myhost"),
	)
	tx.End()
	tracer.Flush(nil)
	fmt.Printf("%+v\n", tracer.Stats())
}

func newSpan(tx *apm.Transaction, start time.Time, name, t, targetType, targetName string) time.Duration {
	span := tx.StartSpanOptions(name, t, apm.SpanOptions{
		ExitSpan: true, Start: start,
	})
	span.Duration = 2 * time.Microsecond
	span.Context.SetServiceTarget(apm.ServiceTargetSpanContext{
		Type: targetType,
		Name: targetName,
	})
	span.End()
	return span.Duration
}

Service.Target.Name empty

package main

import (
	"fmt"
	"time"

	"go.elastic.co/apm/v2"
)

func main() {
	tracer := apm.DefaultTracer()
	tracer.SetSpanCompressionEnabled(true)
	tracer.SetSpanCompressionSameKindMaxDuration(time.Millisecond)
	tracer.SetExitSpanMinDuration(0)
	currentTime := time.Now()
	tx := tracer.StartTransactionOptions("8.7.0", "testplan", apm.TransactionOptions{
		Start: currentTime,
	})
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM users", "mysql", "db", ""),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM catalog", "mysql", "db", ""),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM orders", "mysql", "db", ""),
	)
	tx.End()
	tracer.Flush(nil)
	fmt.Printf("%+v\n", tracer.Stats())
}

func newSpan(tx *apm.Transaction, start time.Time, name, t, targetType, targetName string) time.Duration {
	span := tx.StartSpanOptions(name, t, apm.SpanOptions{
		ExitSpan: true, Start: start,
	})
	span.Duration = 2 * time.Microsecond
	span.Context.SetServiceTarget(apm.ServiceTargetSpanContext{
		Type: targetType,
		Name: targetName,
	})
	span.End()
	return span.Duration
}

Service.Target.Type empty

package main

import (
	"fmt"
	"time"

	"go.elastic.co/apm/v2"
)

func main() {
	tracer := apm.DefaultTracer()
	tracer.SetSpanCompressionEnabled(true)
	tracer.SetSpanCompressionSameKindMaxDuration(time.Millisecond)
	tracer.SetExitSpanMinDuration(0)
	currentTime := time.Now()
	tx := tracer.StartTransactionOptions("8.7.0", "testplan", apm.TransactionOptions{
		Start: currentTime,
	})
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM users", "mysql", "", "myhost"),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM catalog", "mysql", "", "myhost"),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM orders", "mysql", "", "myhost"),
	)
	tx.End()
	tracer.Flush(nil)
	fmt.Printf("%+v\n", tracer.Stats())
}

func newSpan(tx *apm.Transaction, start time.Time, name, t, targetType, targetName string) time.Duration {
	span := tx.StartSpanOptions(name, t, apm.SpanOptions{
		ExitSpan: true, Start: start,
	})
	span.Duration = 2 * time.Microsecond
	span.Context.SetServiceTarget(apm.ServiceTargetSpanContext{
		Type: targetType,
		Name: targetName,
	})
	span.End()
	return span.Duration
}

Target.Name and Target.Type empty

package main

import (
	"fmt"
	"time"

	"go.elastic.co/apm/v2"
)

func main() {
	tracer := apm.DefaultTracer()
	tracer.SetSpanCompressionEnabled(true)
	tracer.SetSpanCompressionSameKindMaxDuration(time.Millisecond)
	tracer.SetExitSpanMinDuration(0)
	currentTime := time.Now()
	tx := tracer.StartTransactionOptions("8.7.0", "testplan", apm.TransactionOptions{
		Start: currentTime,
	})
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM users", "mysql", "", ""),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM catalog", "mysql", "", ""),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM orders", "mysql", "", ""),
	)
	tx.End()
	tracer.Flush(nil)
	fmt.Printf("%+v\n", tracer.Stats())
}

func newSpan(tx *apm.Transaction, start time.Time, name, t, targetType, targetName string) time.Duration {
	span := tx.StartSpanOptions(name, t, apm.SpanOptions{
		ExitSpan: true, Start: start,
	})
	span.Duration = 2 * time.Microsecond
	span.Context.SetServiceTarget(apm.ServiceTargetSpanContext{
		Type: targetType,
		Name: targetName,
	})
	span.End()
	return span.Duration
}

fix: update compressed span name logic for spec change

2c27a44

The logic to infer the compressed span name has been updated to use the service.target.* fields. Update the code to be compliant with the specification.

github-actions bot added the agent-go label Oct 19, 2022

axw reviewed Oct 24, 2022

View reviewed changes

kruskall added 2 commits November 18, 2022 06:29

Merge branch 'main' into fix/update-compressed-span-name

e90ebbf

test: add more composite span name assertions

1665b1b

kruskall requested a review from axw November 18, 2022 05:44

kruskall added 3 commits December 4, 2022 17:01

fix: use the correct field for compressed span name

66e0166

test: add compressed span test cases

3375dcc

Merge branch 'main' into fix/update-compressed-span-name

e250d51

axw approved these changes Dec 5, 2022

View reviewed changes

lint: fix linter issues

fb3d450

kruskall requested a review from axw December 5, 2022 01:17

axw approved these changes Dec 5, 2022

View reviewed changes

kruskall merged commit e0b492f into elastic:main Dec 5, 2022

kruskall deleted the fix/update-compressed-span-name branch December 5, 2022 03:33

lahsivjar mentioned this pull request Feb 8, 2023

8.7 Manual Test Plan elastic/apm-server#10200

Closed

11 tasks

marclop self-assigned this Mar 29, 2023

marclop added the test-plan-ok label Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update compressed span name logic for spec change#1336

fix: update compressed span name logic for spec change#1336
kruskall merged 7 commits intoelastic:mainfrom
kruskall:fix/update-compressed-span-name

kruskall commented Oct 19, 2022

ghost commented Oct 19, 2022 •

edited by ghost

Loading

Build stats

Test stats 🧪

ghost commented Oct 19, 2022 •

edited by ghost

Loading

axw left a comment

kruskall commented Nov 18, 2022

axw commented Nov 18, 2022

kruskall commented Dec 4, 2022

axw Dec 5, 2022

kruskall Dec 5, 2022

axw Dec 5, 2022

marclop commented Mar 29, 2023

Labels

3 participants

Conversation

kruskall commented Oct 19, 2022

ghost commented Oct 19, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

ghost commented Oct 19, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

axw left a comment

Choose a reason for hiding this comment

kruskall commented Nov 18, 2022

axw commented Nov 18, 2022

kruskall commented Dec 4, 2022

axw Dec 5, 2022

Choose a reason for hiding this comment

kruskall Dec 5, 2022

Choose a reason for hiding this comment

axw Dec 5, 2022

Choose a reason for hiding this comment

marclop commented Mar 29, 2023

All target fields populated

Service.Target.Name empty

Service.Target.Type empty

Target.Name and Target.Type empty

Labels

3 participants

ghost commented Oct 19, 2022 •

edited by ghost

Loading

ghost commented Oct 19, 2022 •

edited by ghost

Loading