[exporter/elasticsearch] handle ecs mode mapping #31553
Conversation
Co-authored-by: Andrey Kaipov <andrey.kaipov@target.com>
|
@ycombinator @gregkalapos thanks for the feedback. I hope I addressed the concerns. I wasn't the original dev on this internally, I'm just tying up some loose ends from someone who left us last year. |
|
Thanks @j-kap-t! I reviewed the changes and they look good to me. I'm not an approver, however, so I believe @JaredTan95 will need to review and approve this PR to get it merged. Also, thanks for the additional context re: tying up loose ends. Appreciate you doing that! I'm working on a follow up PR (#31694) where we can expand upon the foundation you're laying here for |
|
@codeboten Is there anything left needed for approval & merge? Thanks! |
|
@JaredTan95 fixed. |
|
@JaredTan95 @codeboten @djaglowski is there anything left needed to merge? |
|
@open-telemetry/collector-contrib-maintainer This PR has the component approver's approval and so I think is ready to merge now, whenever you get a chance. Thanks! |
**Description:** This PR proposes adding @ycombinator as a codeowner for the `elasticsearch` exporter component, being an [employee of Elastic](https://www.linkedin.com/company/elastic-co/people/?keywords=shaunak) and also meeting the codeowner [requirements](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#requirements): 1. [Be a member of the OpenTelemetry organization.](https://github.com/open-telemetry/community/blob/main/community-membership.md#member) * https://github.com/orgs/open-telemetry/people?query=ycombinator 2. (Code Owner Discretion) It is best to have resolved an issue related to the component, contributed directly to the component, and/or review component PRs. How much interaction with the component is required before becoming a Code Owner is up to any existing Code Owners. * Resolved #26647 via #29619 * Reviewed #31553 * Contributed #31694 as follow up to #31553 * Reviewed #31848
|
|
||
| document.AddString("log.level", record.SeverityText()) | ||
|
|
||
| if record.Body().Type() == pcommon.ValueTypeStr { |
There was a problem hiding this comment.
Does this mean that if body of the log is not a string, it is dropped? Is this the intended behavior?
There was a problem hiding this comment.
Oh I can see a previous comment about this: #31553 (comment).
I'm not sure if handing other body types in another PR is good enough, given that the component's stability is "Beta".
Can we at least create a separate issue to fix this after this PR is merged (hopefully soon)?
There was a problem hiding this comment.
Looking more into this, it seems there might be more changes coming in future PRs to how the ecs mapping mode works. Is this a reasonable assumption?
If yes, I propose to add a note in the README warning users that the ecs mapping mode is currently not stable. This note could be removed after this mode stabilizes.
What do you think @ycombinator @j-kap-t ?
There was a problem hiding this comment.
@astencel-sumo Yes, there is at least one follow up PR in the works for ecs mapping mode already: #31694. And there might be others too in the future. So I like the idea of adding a note to the README warning users that ecs mapping mode is experimental or something to that effect as part of this PR.
Co-authored-by: Andrzej Stencel <astencel@sumologic.com>
Co-authored-by: Andrzej Stencel <astencel@sumologic.com>
|
Thanks for the updates @j-kap-t . Can you also update the README, adding a note to the |
….mode: ecs` (#31694) **Description:** <Describe what has changed.> This PR is a follow up to #31553. In that PR, the foundations were laid for encoding log records with ECS fields when the user specifies `mapping.mode: ecs` in their exporter configuration. This PR adds ECS conversions for several additional fields. I expect there will be more followup PR(s) to add more conversions, especially ones that might not be straightforward. **Link to tracking issue:** #29742 **Testing:** Added unit tests **Documentation:** The `mapping.mode: ecs` configuration setting is already [documented](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter#configuration-options). --------- Co-authored-by: Andrzej Stencel <andrzej@andrzejstencel.pl>
Description:
Initial pass in implementing the ecs mapping mode that is documented but not implemented. This only covers a small subset of attribute mappings from otel > ecs, but the core record fields should be properly mapped.
Link to tracking Issue:
Testing:
Added test for ecs mapping mode.
Documentation:
Updated the documentation to reflect the default behavior is the current behavior with no mapping applied.
Reopening #30454