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

Update OTLP span -> DD span logic for operation and resource name #30616

Merged
merged 6 commits into from
Nov 6, 2024

Conversation

IbraheemA
Copy link
Contributor

@IbraheemA IbraheemA commented Oct 30, 2024

What does this PR do?

Update the logic for determining DD span operation name and resource name from OTLP spans to produce shorter names and better align with the OTel spec.

The operation name logic is based on the logic used in dd-trace-go

Minor changes are made to resource name to bring the resource name closer in line with the OTel specification for span name, since we would like the two concepts to be equivalent.

This is a breaking change, so it is gated for now - users can enable the new logic by setting enable_operation_and_resource_name_logic_v2 in apm_config.features. In this first version, if a user has the new logic enabled along with SpanNameAsResourceName and SpanNameRemappings, the latter will override the former and revert to the v1 logic. This is intended to make sure users know what they're doing by enabling the new breaking logic.

If a user hasn't enabled the new logic or has overridden it with another config option, surface a warning encouraging them to switch and informing them how. In a future PR, these will be cleaned up and enable_operation_and_resource_name_logic_v2 will become the default.

Once we merge this and update the agent version in opentelemetry-collector-contrib, it will be available for datadogexporter as well. So, I'll create an issue in that repo to explain the utility of the update and encourage users to enable it.

The RFC with more information can be found here

Describe how to test/QA your changes

Set enable_operation_and_resource_name_logic_v2 in apm_config.features and run the agent + trace-agent. Send some spans and verify that the resource name and operation name are set in the new way (e.g., there is no io.opentelemetry... prefix on operation name, an HTTP server request from the OTel java calendar app would have operation name http.server.request.)

@IbraheemA IbraheemA requested review from a team as code owners October 30, 2024 15:35
@IbraheemA IbraheemA requested a review from mx-psi October 30, 2024 15:35
@agent-platform-auto-pr
Copy link
Contributor

agent-platform-auto-pr bot commented Oct 30, 2024

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=48382721 --os-family=ubuntu

Note: This applies to commit 985bcee

Copy link
Contributor

@drichards-87 drichards-87 left a comment

Choose a reason for hiding this comment

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

Left a suggestion from Docs and approved the PR.

@github-actions github-actions bot added team/opentelemetry OpenTelemetry team long review PR is complex, plan time to review it labels Oct 30, 2024
pkg/trace/traceutil/otel_util.go Outdated Show resolved Hide resolved
pkg/trace/traceutil/otel_util.go Show resolved Hide resolved
isRPC := rpcValue != ""
isAws := isRPC && (rpcValue == "aws-api")
// AWS client
if isAws && isClient {
Copy link
Member

Choose a reason for hiding this comment

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

do we need to handle aws server ?

Copy link
Contributor Author

@IbraheemA IbraheemA Nov 4, 2024

Choose a reason for hiding this comment

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

Could you explain how you think we should handle it? I didn't see it discussed in the document specifying conventions for operation names, and it's not present in the existing tracer code

I could set it to aws.server.request, just not sure if ".request" would always be appropriate.

Copy link

Regression Detector

@IbraheemA
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented Nov 6, 2024

🚂 MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

@dd-devflow
Copy link

dd-devflow bot commented Nov 6, 2024

🚂 MergeQueue: pull request added to the queue

The median merge time in main is 23m.

Use /merge -c to cancel this operation!

@dd-mergequeue dd-mergequeue bot merged commit 47294c1 into main Nov 6, 2024
208 of 217 checks passed
@dd-mergequeue dd-mergequeue bot deleted the ibraheem/operation-and-resource-name-update-v2 branch November 6, 2024 19:58
@github-actions github-actions bot added this to the 7.61.0 milestone Nov 6, 2024
@IbraheemA IbraheemA added the qa/done QA done before merge and regressions are covered by tests label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
long review PR is complex, plan time to review it qa/done QA done before merge and regressions are covered by tests team/opentelemetry OpenTelemetry team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants