Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[exporter/elasticsearch] Add OTel mapping mode for traces #34472
Changes from 3 commits
e7f41d2
07ac2e7
e8337c7
66ee18f
b2cec62
d19eebf
fd5da92
9d6d3db
7c6f811
b45df76
fb69873
3130cf1
54925f4
71cc306
f6416f1
9b8588e
deb70c4
f18b053
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:There was a problem hiding this comment.
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, andstripDataStreamAttributes
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function call is simple enough to inline and would also be better for readability. So, my vote is to inline.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, unlikeresource
.But I'll get back to the team and see if there is any implication on data correctness.
There was a problem hiding this comment.
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