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.Geneva] Avoid allocation for serializing scopes #818

Merged
merged 12 commits into from
Dec 19, 2022

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Dec 12, 2022

Changes

  • Avoid allocation for serializing scopes
  • Appropriate CHANGELOG.md updated for non-trivial changes

Benchmark Results:

main branch:

Method Mean Error StdDev Gen0 Allocated
SerializeLogRecord 660.2 ns 6.22 ns 5.82 ns 0.0162 104 B
Export 694.2 ns 2.49 ns 2.21 ns 0.0162 104 B

With this PR:

Method Mean Error StdDev Gen0 Allocated
SerializeLogRecord 582.3 ns 0.81 ns 0.72 ns - -
Export 646.0 ns 1.10 ns 0.86 ns - -

@utpilla utpilla requested a review from a team December 12, 2022 20:30
@utpilla utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Dec 12, 2022
@@ -33,11 +33,12 @@ internal sealed class MsgPackLogExporter : MsgPackExporter, IDisposable
private readonly IReadOnlyDictionary<string, object> m_customFields;

private readonly ExceptionStackExportMode m_exportExceptionStack;
private readonly ExporterStateForScopes exporterStateForScopes;
Copy link
Member

Choose a reason for hiding this comment

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

nit: m_exporterStateForScopes to be consistent with the other fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I think we should update the field names to use Pascal case instead though. 😄

@@ -127,6 +128,8 @@ public MsgPackLogExporter(GenevaExporterOptions options)
this.m_customFields = customFields;
}

this.exporterStateForScopes = new ExporterStateForScopes() { CustomFields = this.m_customFields };
Copy link
Member

Choose a reason for hiding this comment

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

nit: Just reading the diff, it doesn't seem like ExporterStateForScopes is needed. You could do something more or less like this...

class MsgPackLogExporter
{
    private readonly ThreadLocal<SerializationDataForScopes> m_serializationData = new(() => null);

    private static readonly Action<LogRecordScope, MsgPackLogExporter> ProcessScopeForIndividualColumns = (scope, state) =>
    {
        var stateData = state.m_serializationData.Value;
        var customFields = state.m_customFields;

        // Do work
    }
}

Basically use MsgPackLogExporter as the state type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! Updated.

@github-actions github-actions bot requested a review from CodeBlanch December 15, 2022 05:06
@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #818 (cf0c5bd) into main (3768220) will increase coverage by 0.35%.
The diff coverage is 61.65%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
+ Coverage   77.55%   77.90%   +0.35%     
==========================================
  Files         168      178      +10     
  Lines        5243     5459     +216     
==========================================
+ Hits         4066     4253     +187     
- Misses       1177     1206      +29     
Impacted Files Coverage Δ
...stana/Implementation/InstanaExporterEventSource.cs 0.00% <0.00%> (ø)
...er.Instana/Implementation/InstanaSpanSerializer.cs 89.62% <0.00%> (-4.17%) ⬇️
...metry.Exporter.Instana/Implementation/Transport.cs 18.29% <0.00%> (-2.76%) ⬇️
...ry.Exporter.Geneva/Metrics/GenevaMetricExporter.cs 58.49% <100.00%> (+0.39%) ⬆️
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 94.19% <100.00%> (+1.55%) ⬆️
...ter.Geneva/MsgPackExporter/MsgPackTraceExporter.cs 92.92% <100.00%> (+0.03%) ⬆️
...etry.Exporter.Instana/Implementation/SpanSender.cs 61.53% <100.00%> (ø)
...Implementation/AspNetInstrumentationEventSource.cs 76.92% <0.00%> (ø)
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <0.00%> (ø)
... and 9 more

@github-actions github-actions bot requested a review from CodeBlanch December 15, 2022 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants