-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 mapping.mode: raw
configuration option
#29619
Merged
MovieStoreGuy
merged 17 commits into
open-telemetry:main
from
ycombinator:exp-es-omit-attributes-prefix
Feb 6, 2024
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
c28e23a
Document new `mapping.omit_attributes_prefix` config option
ycombinator c5d7941
Define new `mapping.omit_attributes_prefix` config option
ycombinator a6ff601
Add ability to merge documents
ycombinator 233e552
Use new `mapping.omit_attributes_prefix` option while encoding
ycombinator 30ca34a
Adding CHANGELOG entry
ycombinator 5d37b81
Update unit tests for config
ycombinator 51bef51
Simplify implementation
ycombinator 161ebdb
Add unit test
ycombinator 115f332
Update CHANGELOG entry
ycombinator a870aaf
Add raw mapping mode
ycombinator 0139e75
Update README
ycombinator d7b8b4e
Replace mapping.omit_attributes_prefix: true with mapping.mode: raw
ycombinator 13ed3db
Adding separate test case for format: raw
ycombinator a6f6146
Escape text in CHANGELOG entry
ycombinator c9f6fd5
make gci
andrzej-stencel 1ec5255
Fix unit tests
ycombinator e13b484
Merge branch 'main' into exp-es-omit-attributes-prefix
andrzej-stencel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: enhancement | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) | ||
component: elasticsearchexporter | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: "Add `mapping.mode: raw` configuration option" | ||
|
||
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
issues: [26647] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | | ||
Setting `mapping.mode: raw` in the Elasticsearch exporter's configuration | ||
will result logs and traces being indexed into Elasticsearch with their | ||
attribute fields directly at the root level of the document instead inside an | ||
`Attributes` object. Similarly, this setting will also result in traces being | ||
indexed into Elasticsearch with their event fields directly at the root level | ||
of the document instead of inside an `Events` object. | ||
|
||
# If your change doesn't affect end users or the exported elements of any package, | ||
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Is it possible for a user to want both "ecs" and "raw" functionality? With this configuration, this is not possible.
Looking at the code, I'm not sure the "ecs" mapping mode actually does anything? Is there a functional difference between "none" and "ecs" mapping mode in the current state of the exporter?
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.
Yeah, I don't see mapping mode being used at all (before this PR introduced
raw
). So I don't think there's any functional difference betweennone
andecs
mapping modes today.I looked at the PR that introduced the
ecs
mapping mode — #2324 — but I don't see any explanation for why it is functionally the same asraw
. My guess is thatecs
was introduced as part of some future-proofing work that was never done?Given that
ecs
mapping mode doesn't really do anything today, I can't say that users may want bothecs
andraw
functionality at the same time. Perhaps we should deprecateecs
(in a separate PR) and later remove it, and then go ahead with this PR introducingraw
mapping mode? If so, are there any guidelines around such deprecation and removal (I'm new to the OTel community)?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 makes sense. If the
ecs
mode does not do anything, then there's no point in enabling bothecs
andraw
at the same time.Agree 💯
I think both changes can be done independently, can't they? We can move on with introducing
raw
mode in this PR and separately create another PR to deprecateecs
. To put it yet another way,ecs
removal does not block introduction ofraw
in my view.As to the deprecation and removal process, the
ecs
mode is currently the default, so I think the following steps make sense:ecs
mode and at the same time change the default fromecs
tonone
- this is a no-op change assuming both modes are functionally the same.ecs
mode after a long time - perhaps 3 months, perhaps 6 months - I suppose there's no rush here, because having this option doesn't really hurt us, right?I don't think introducing a feature gate for this removal makes sense, given that the fixing the configuration - removing
mode: ecs
or changing it tomode: none
- is simpler than using a feature gate.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.
Thanks @astencel-sumo, I've created #30441 to track the deprecation of
mode: ecs
and changing the default tomode: none
.I agree we can just go ahead with this PR here introducing
mode: raw
now itself, independently of what becomes ofmode: ecs
.