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 exporter put log resource in root instead of under 'resources' #3131

Conversation

ShadowMaxLeb
Copy link
Contributor

@ShadowMaxLeb ShadowMaxLeb commented Nov 6, 2024

In order to give the user full flexibility on what and where to put resources for logs, remove the fact that it was forced to always be under the resource.
This allows to have resources such as client.id that will be under the root and compliant with ECS format.

Closes #3119

Changes

Log resources are set under the root object and not under resources anymore.

@ShadowMaxLeb ShadowMaxLeb requested a review from a team as a code owner November 6, 2024 15:19
Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 1a0d85e
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/672e1feda590e20008428968

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.85%. Comparing base (497eaf4) to head (1a0d85e).
Report is 159 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3131      +/-   ##
==========================================
+ Coverage   87.12%   87.85%   +0.73%     
==========================================
  Files         200      195       -5     
  Lines        6109     6136      +27     
==========================================
+ Hits         5322     5390      +68     
+ Misses        787      746      -41     

see 100 files with indirect coverage changes

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@ShadowMaxLeb ShadowMaxLeb force-pushed the exporter-elasticsearch-resources branch from 84ba218 to d752666 Compare November 8, 2024 09:05
@marcalff
Copy link
Member

marcalff commented Nov 8, 2024

@ShadowMaxLeb

In the CHANGELOG.md, please add an entry for important changes, to document that resources are no longer added under "root/resource" but are added under root instead.

I will merge this PR after that.

Thanks for the fix.

@marcalff marcalff added the important change Important change, that should be documented in CHANGELOG label Nov 8, 2024
@ShadowMaxLeb ShadowMaxLeb force-pushed the exporter-elasticsearch-resources branch from d752666 to 1a0d85e Compare November 8, 2024 14:27
@ShadowMaxLeb
Copy link
Contributor Author

@marcalff done

@marcalff
Copy link
Member

marcalff commented Nov 8, 2024

@marcalff done

Thanks.

I meant a section similar to this:

Important changes:

* [API] Jaeger Propagator should not be deprecated
  [#3086](https://github.com/open-telemetry/opentelemetry-cpp/pull/3086)

  * Deprecation of the Jaeger propagator, as announced on 2023-01-31
    in version 1.8.2, is now reverted.
  * This deprecation turned out to be not justified,
    as the Jaeger propagator can be used without the (now removed)
    Jaeger exporter.

but since there is a note in the changelog, I will notice it and edit it when doing the next release, so this will do for now.

Merging.

@marcalff marcalff added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Nov 8, 2024
@marcalff marcalff merged commit 24a523c into open-telemetry:main Nov 8, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
important change Important change, that should be documented in CHANGELOG ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elasticsearch Log exporter "resources" are under "resources" key while it should be under the "root"
3 participants