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

PassThru TableNameMappings using the logger category name. #345

Merged
merged 22 commits into from
May 24, 2022

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented May 6, 2022

Related to:
#289
When the PassThruTableNameMappings rule was enable, the best fit table name given the category name were being generated while serializing each LogRecord.

Next steps:

  • Add benchmarks to compare perf w/ and w/o cache.
    [Not for merging] Benchmarks for PassThruTableMappings. Yun-Ting/opentelemetry-dotnet-contrib#1
    PassThru TableNameMappings using the logger category name. #288 (comment)

    baseline (main branch)
    * Total Loops: 1,106,043,741
    * Average Loops/Second: 3,057,844
    * Average CPU Cycles/Loop: 6,867
    
    No Cache with passThruTableMappings Enabled
    * Total Loops: 878,389,049
    * Average Loops/Second: 2,912,971
    * Average CPU Cycles/Loop: 7,193
    
    With Cache with passThruTableMappings Enabled
    * Total Loops: 889,221,235
    * Average Loops/Second: 2,904,167
    * Average CPU Cycles/Loop: 7,196
    
  • Document use cases for this feature. => Will be addressed in a follow-up PR.

  • Add testcase for Linux environment.

  • Determine the behavior of Sanitize when the category name is invalid. => When the category name is invalid, eventName will be assigned as null. GenevaExporter will not pick up events with null eventNames.

@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #345 (d849ec8) into main (0a03cfc) will increase coverage by 0.74%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   21.96%   22.70%   +0.74%     
==========================================
  Files         144      144              
  Lines        4421     4465      +44     
==========================================
+ Hits          971     1014      +43     
- Misses       3450     3451       +1     
Impacted Files Coverage Δ
...penTelemetry.Exporter.Geneva/GenevaBaseExporter.cs 96.77% <80.00%> (-1.48%) ⬇️
...OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs 91.46% <100.00%> (+1.58%) ⬆️
...Telemetry.Exporter.Geneva/MessagePackSerializer.cs 92.75% <100.00%> (+0.21%) ⬆️

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

Approved with some suggestions.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with a changelog entry.

@github-actions github-actions bot requested a review from CodeBlanch May 24, 2022 00:08
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