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

Add new attribute mappings #139

Merged
merged 4 commits into from
Sep 12, 2023
Merged

Conversation

stevejgordon
Copy link
Contributor

@stevejgordon stevejgordon commented Aug 10, 2023

Adds support for new attributes defined in more recent semantic conversion schema versions. This initial set is needed to support attributes emitted by the .NET Elasticsearch client instrumentation, which conforms to https://opentelemetry.io/schemas/1.21.0. The go.opentelemetry.io/collector/semconv/v1.5.0 package does not include these attributes, and the latest version only includes up to 1.18.0. For now, I've included the new attributes directly in our mapping code and updated them to ensure both old and new attribute names are supported. In the long term, we need to discuss how we handle other new attributes and have a strategy for mapping new schema versions as they are made stable and released.

@stevejgordon stevejgordon marked this pull request as ready for review August 29, 2023 13:25
@stevejgordon stevejgordon requested a review from a team as a code owner August 29, 2023 13:25
kruskall
kruskall previously approved these changes Aug 29, 2023
@stevejgordon
Copy link
Contributor Author

Thanks for the approval, @kruskall. I rebased this so that the commit was signed. What's the process for getting this merged?

@kruskall
Copy link
Member

@stevejgordon 👋

There's no specific process, once the PR is merged the dependency in apm-server will be bumped and the changes included in the next release.
Code looks code but I'd like another pair of eyes to approve this to make sure this is the direction we want to go as we haven't decided on how to deal with different versions of otel semconv (#116)

@kruskall kruskall dismissed their stale review August 29, 2023 20:59

removing approval to avoid merging this by mistake

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.

@stevejgordon I think the changes for TranslateSpan are fine, but there's a couple of issues in TranslateTransaction. Can you please check those? The server.* fields are mentioned in the 1.21 schema here: https://github.com/open-telemetry/semantic-conventions/blob/4bbb8c907402caa90bc077214e8a2c78807c1ab9/schemas/1.21.0#L15-L19

Adding attributes for a newer semver version is fine as long as we keep support for the older version. Longer term we need to figure out something sustainable, but we don't need to solve that today.

input/otlp/traces.go Outdated Show resolved Hide resolved
input/otlp/traces.go Outdated Show resolved Hide resolved
@stevejgordon stevejgordon requested a review from axw August 30, 2023 13:40
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.

@stevejgordon thanks, LGTM - just one minor comment.

input/otlp/traces.go Outdated Show resolved Hide resolved
@axw axw enabled auto-merge (squash) September 12, 2023 09:31
@axw axw merged commit 9b626b8 into elastic:main Sep 12, 2023
2 checks passed
@stevejgordon stevejgordon deleted the add-new-attribute-mappings branch April 15, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants