-
Notifications
You must be signed in to change notification settings - Fork 440
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] Fix forward protocol encoding for ETW exporter #2473
[EXPORTER] Fix forward protocol encoding for ETW exporter #2473
Conversation
This reverts commit 63aa87d.
@ThomsonTan - Can you add some description about the changes, will help review this better :) |
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.
LGTM. Thanks for the fix.
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. |
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.opentelemetry-cpp/exporters/etw/include/opentelemetry/exporters/etw/etw_provider.h
Line 370 in c4f39f2
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