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.

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: handle span compression with new service_target_* fields #1339

Merged
merged 5 commits into from
Nov 28, 2022

Conversation

kruskall
Copy link
Member

Update compression logic to use service target fields and stop relying on resource internally.

The updated logic is compliant with the spec on span compression.
See https://github.com/elastic/apm/blob/main/specs/agents/handling-huge-traces/tracing-spans-compress.md#consecutive-same-kind-compression-strategy

Closes #1292

Update compression logic to use service target fields and stop
relying on resource internally.
@apmmachine
Copy link
Contributor

apmmachine commented Oct 23, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-28T10:21:23.346+0000

  • Duration: 59 min 37 sec

Test stats 🧪

Test Results
Failed 0
Passed 8688
Skipped 215
Total 8903

🤖 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
Copy link
Contributor

apmmachine commented Oct 23, 2022

🌐 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) 👍
Lines 82.252% (11456/13928) 👍 0.073
Conditionals 100.0% (0/0) 💚

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

I'm surprised that no tests have changed. Do we have any tests for "same kind" compression with target service?

@simitt
Copy link
Contributor

simitt commented Nov 14, 2022

@kruskall have you had time to follow up with the question regarding the tests?

@kruskall
Copy link
Member Author

I'm surprised that no tests have changed. Do we have any tests for "same kind" compression with target service?

Sorry for the late reply. I'm not sure what should change. We have tests to verify "same kind" compression is working as intended (https://github.com/elastic/apm-agent-go/blob/main/span_test.go#L536-L656) but they still pass since there is no "change" in behaviour.
Service target fields are fully supported and inferred when possible, same as the old destination service. Moving to the new fields would not change the "same kind" relantionship between spans created the same way.

@kruskall kruskall requested a review from axw November 18, 2022 05:28
@axw
Copy link
Member

axw commented Nov 21, 2022

It seems to me we have a (pre-existing) gap in our testing: AFAICS the "same kind" compression tests don't cover the code path where the span type and subtype are the same, but the target service name/type differ. I expected we would have a test for this, and that we should have changed the test to set service target rather than the deprecated destination fields.

We can merge this as is, but it would be good to improve the tests.

…different service target fields

Add a test case that ensure spans with different service target fields
are not compressed even when they are the same kind.
The service target fields are inferred by the db context.
@kruskall
Copy link
Member Author

It seems to me we have a (pre-existing) gap in our testing: AFAICS the "same kind" compression tests don't cover the code path where the span type and subtype are the same, but the target service name/type differ. I expected we would have a test for this, and that we should have changed the test to set service target rather than the deprecated destination fields.

We can merge this as is, but it would be good to improve the tests.

Good point! I think we can add it here since it's related to the change and it's fairly small. I've added a test case, let me know what you think! 👍

@kruskall kruskall requested a review from axw November 21, 2022 05:08
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we could simplify the test a little bit by not relying on inference.

span_test.go Show resolved Hide resolved
@kruskall kruskall requested a review from axw November 21, 2022 09:05
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks great, thank you :)

@kruskall kruskall enabled auto-merge (squash) November 21, 2022 10:49
@kruskall kruskall merged commit 79b0f97 into elastic:main Nov 28, 2022
@kruskall kruskall deleted the refactor/service-target-compression branch December 5, 2022 01:11
@marclop marclop self-assigned this Mar 28, 2023
@marclop
Copy link
Contributor

marclop commented Mar 29, 2023

Verified with 8.7.0 BC9 and the latest main (332875e):

package main

import (
	"fmt"
	"time"

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

func main() {
	tracer := apm.DefaultTracer()
	tracer.SetSpanCompressionEnabled(true)
	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 users", "mysql", "db", "myhost"),
	)
	currentTime.Add(
		newSpan(tx, currentTime, "SELECT * FROM orders", "mysql", "db", "anotherhost"),
	)
	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
}

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle span compression with new service_target_* fields
5 participants