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

[exporter/elasticsearch] Add OTel mapping mode for traces #34472

Merged

Conversation

carsonip
Copy link
Contributor

@carsonip carsonip commented Aug 7, 2024

Description:

Add OTel mapping mode for traces.

OTel mapping mode is a new mapping mode described in https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter#elasticsearch-document-mapping

Update logs and metrics OTel mapping mode to always emit "scope" and zero int, but not emit empty strings for known fields.

Breaking change to remove trace_flags from logs.

Link to tracking Issue:
Fixes #34588 #34590

Testing:
Added exporter tests

Documentation:

@carsonip carsonip force-pushed the otel-mode-traces-independent branch 2 times, most recently from 956ef23 to 7e8d0a0 Compare August 8, 2024 16:07
@carsonip carsonip force-pushed the otel-mode-traces-independent branch from 7e8d0a0 to e7f41d2 Compare August 8, 2024 16:31
@carsonip carsonip marked this pull request as ready for review August 8, 2024 16:40
@carsonip carsonip requested review from a team and mx-psi August 8, 2024 16:40
exporter/elasticsearchexporter/model.go Outdated Show resolved Hide resolved
exporter/elasticsearchexporter/model.go Outdated Show resolved Hide resolved
Comment on lines 179 to 180
addDataStreamAttributes(&document, attributeMap)
stripDataStreamAttributes(attributeMap)
Copy link
Member

@lahsivjar lahsivjar Aug 9, 2024

Choose a reason for hiding this comment

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

nit: pcommon.Map#Get could get expensive with more attributes. Though I don't think we would have a huge number of keys for it to be a conecern but it is not helping with readability of the code either. Also, I am not a fan of these util functions. How about:

	attributeMap.RemoveIf(func(k string, v pcommon.Value) bool {
		// just a few keys in datastream keys
		if slices.Contains(datastreamKeys, k) {
			// Add attribute will not add empty values
			document.AddAttribute(k, v)
			return true
		}
		return false
	})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it is not great. It was designed as such as we need to call addDataStreamAttributes for record level attributes, and stripDataStreamAttributes for all resource, scope, and record level attributes. Splitting them into 2 functions makes it easier to understand. Performance would have been fine if attributes Get was O(1), but the current implementation isn't.

Would you still prefer implementing it inline for all 3 kinds of attributes?

Copy link
Member

Choose a reason for hiding this comment

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

Would you still prefer implementing it inline for all 3 kinds of attributes?

I think the function call is simple enough to inline and would also be better for readability. So, my vote is to inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, looking much cleaner now.

Copy link
Member

@lahsivjar lahsivjar left a comment

Choose a reason for hiding this comment

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

Gave a quick look, a couple of comments for code readability. Will give another look once the PR is updated with the changes after the dependent PR is merged.

exporter/elasticsearchexporter/model.go Show resolved Hide resolved
Copy link
Member

@lahsivjar lahsivjar left a comment

Choose a reason for hiding this comment

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

LGTM! A non-blocker question about trace flags.

Separate from the PR, we need to refactor the objmodel and model as it is getting harder to reason about these.

exporter/elasticsearchexporter/model.go Show resolved Hide resolved
component: elasticsearchexporter

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add OTel mapping mode for traces, update OTel mapping mode for logs
Copy link
Contributor

Choose a reason for hiding this comment

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

same feedback as on the metrics PR, any chance you can add more to this note to explain what this change is about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OTel mapping mode, aka OTel native mapping mode, is a new mapping mode that is actively under development, where it maps to a model that's very close to the actual OTel data model, unlike ECS mode where a lot of translations are done in elasticsearchexporter. See elasticsearchexporter docs for details. We don't have public docs about mapping that we can point to yet, as it is all under active development. It requires Elasticsearch native support to magically keep the existing UIs working.

There's an initial implementation for logs (done in #33290) as you may have noticed, and here comes the traces (this PR) and metrics (#34248) counterparts.

if scopeSchemaURL != "" {
scopeMap.PutStr("schema_url", scopeSchemaURL)
}
if scope.DroppedAttributesCount() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a difference of treatment compared to the resource level. Don't you want to set this always even if zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. I think it was initially implemented in this manner in a previous PR for efficiency: #33290 . Also, scope is skipped entirely if there is nothing interesting, unlike resource.

But I'll get back to the team and see if there is any implication on data correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to keep the handling consistent

@carsonip carsonip requested a review from atoulme August 19, 2024 10:11
@andrzej-stencel andrzej-stencel merged commit f0d7e7f into open-telemetry:main Aug 23, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 23, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
…etry#34472)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Add OTel mapping mode for traces.

OTel mapping mode is a new mapping mode described in
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter#elasticsearch-document-mapping

Update logs and metrics OTel mapping mode to always emit "scope" and
zero int, but not emit empty strings for known fields.

Breaking change to remove `trace_flags` from logs.

**Link to tracking Issue:** <Issue number if applicable>
Fixes open-telemetry#34588 open-telemetry#34590 

**Testing:** <Describe what testing was performed and which tests were
added.>
Added exporter tests

**Documentation:** <Describe the documentation added.>
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.

Update Span Kind string values to match collector's current values
7 participants