Skip to content

Conversation

ThomsonTan
Copy link
Contributor

@ThomsonTan ThomsonTan commented Jan 2, 2024

The ETW exporter has code path for MsgPack serialization with macro HAVE_MSGPACK, but it stores all payload to an JSON object then serializes it to MsgPack as below which is incorrect.

std::vector<uint8_t> v = nlohmann::json::to_msgpack(jObj);

The payload should follow fluent protocol as it is currently done in the fluent exporter in the contrib repo as below.

https://github.com/open-telemetry/opentelemetry-cpp-contrib/blob/main/exporters/fluentd/src/log/fluentd_exporter.cc#L95

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (423ecac) 87.09% compared to head (bde8fb2) 87.09%.

Additional details and impacted files

Impacted file tree graph

@@ Coverage Diff @@ ## main #2473 +/- ## ======================================= Coverage 87.09% 87.09% ======================================= Files 200 200 Lines 6103 6103 ======================================= Hits 5315 5315 Misses 788 788 
@ThomsonTan ThomsonTan marked this pull request as ready for review January 3, 2024 17:53
@ThomsonTan ThomsonTan requested a review from a team January 3, 2024 17:53
@lalitb
Copy link
Member

lalitb commented Jan 3, 2024

@ThomsonTan - Can you add some description about the changes, will help review this better :)

@marcalff marcalff changed the title Fix forward protocol encoding for ETW exporter [EXPORTER] Fix forward protocol encoding for ETW exporter Jan 10, 2024
@lalitb lalitb self-assigned this Jan 16, 2024
Copy link
Member

@lalitb lalitb 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.

@marcalff
Copy link
Member

Patch written by @ThomsonTan with approval from @lalitb (2 maintainers), small size, and localized to ETW.

This is sufficient to merge in my opinion, adding ok-to-merge label.

@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 Jan 17, 2024
@marcalff marcalff merged commit f3132e5 into open-telemetry:main Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)

3 participants