-
Notifications
You must be signed in to change notification settings - Fork 306
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
[Geneva] Optimize metrics exporter serializer #2460
base: main
Are you sure you want to change the base?
Conversation
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Should I update the changelog also or what else is missing from the PR? |
@TimothyMothra, @xiang17, FYI |
I will review the PR in couple of days. |
There is a test failure, could you please check? |
@@ -46,18 +41,12 @@ public unsafe void Send(MetricEventType eventType, byte[] data, int size) | |||
#endif | |||
public unsafe void SendOtlpProtobufEvent(byte[] data, int size) | |||
{ | |||
if (this.IsEnabled()) | |||
EventData descr = default; | |||
fixed (byte* blob = &data[0]) |
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.
Wondering why this check had been removed here if (data != null && data.Length != 0)
@pentp First of all, thank you for your hard work on this PR! I really appreciate the effort you’ve put into improving the Geneva Exporter. I can see this PR is bringing meaningful improvements. However, this PR contains a mix of unrelated changes, which makes it quite complex to review in a single pass. To facilitate an easier and more effective review, I’d recommend breaking it into multiple PRs. I have reviewed the following files, which contain miscellaneous improvements that can remain in one PR:
For better maintainability and clarity, I suggest moving the Thanks again for your contribution! Looking forward to the updated PRs. |
Higher-level changes include serializing resource level tags only once for
OtlpProtobufMetricExporter
.Most of the changes are just low-level optimizations for efficient buffer access, single-pass string encoding, etc.