fix: mark spans representing calls to external services as exit spans #1317
Conversation
Exit spans are spans that describe a call to an external service, such as an outgoing HTTP request or a call to a database. Marking a span as an exit span also triggers service target fields and destination fields inference. Update tests to reflect the new fields. Disable compression in some tests as exit spans can be aggregated based on certain fields, leading to unexpected results. Additionally, make sure exit spans are not being used as parents of other spans as that would cause the spans to be dropped.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
|
/test |
module/apmgormv2/apmgorm_test.go
Outdated
| } | ||
| assert.Equal(t, dsnInfo.Address, span.Context.Destination.Address) | ||
| assert.Equal(t, dsnInfo.Port, span.Context.Destination.Port) | ||
| assert.Equal(t, dsnInfo.User, span.Context.Destination.Service.Name) |
There was a problem hiding this comment.
| assert.Equal(t, dsnInfo.User, span.Context.Destination.Service.Name) | |
| assert.Equal(t, dsnInfo.Database, span.Context.Destination.Service.Name) |
Right?
There was a problem hiding this comment.
This is actually destination service name, the old deprecated field. It should use the driver name.
Co-authored-by: Andrew Wilkins <[email protected]>
this was not a service target field but the old deprecated destination service name. The field value is expected to be the driver name. Update the test to reflect what the implementation is doing.
trace request are now non-exit spans while the parent span is always an exit span.
|
This uncovered another bug with the agent which is not respecting the spec. Blocked by #1320 |
PR merged. Unblocking |
There was a problem hiding this comment.
LGTM. I think the integration tests need updating: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-go%2Fapm-agent-go-mbp/detail/PR-1317/8/pipeline
The driver name is changing during integration tests so we can't hardcode sqlite3. Update the test to pass the driver name from the testcase and compare it to the expected result.
🌐 Coverage report
|
|
There's a few cases where we're not marking spans as exit spans, and I found a panic related to ending a child of an exit span. I'll open a PR. |
Exit spans are spans that describe a call to an external service, such as an outgoing HTTP request or a call to a database.
Marking a span as an exit span also triggers service target fields and destination fields inference.
Update tests to reflect the new fields.
Disable compression in some tests as exit spans can be aggregated based on certain fields, leading to unexpected results.
Additionally, make sure exit spans are not being used as parents of other spans as that would cause the spans to be dropped.
Closes #1315