Skip to content

[Geneva] Do not cache long logger names - #4684

Merged
martincostello merged 2 commits into
open-telemetry:mainfrom
martincostello:geneva-fix-cache-retention
Jul 9, 2026
Merged

martincostello merged 2 commits into
open-telemetry:mainfrom
martincostello:geneva-fix-cache-retention

Conversation

@martincostello

Copy link
Copy Markdown
Member

Changes

Apply an upper limit to the length of cached sanitized logger names added in #4498.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

Apply an upper limit to the length of cached sanitized logger names.
Comment thread src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md Outdated
@github-actions github-actions Bot added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Jul 9, 2026
@martincostello
martincostello marked this pull request as ready for review July 9, 2026 07:15
@martincostello
martincostello requested a review from a team as a code owner July 9, 2026 07:15
Copilot AI review requested due to automatic review settings July 9, 2026 07:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens the Geneva exporter’s sanitized logger category-name cache by introducing a maximum input length for cache entries, preventing excessively long logger names from being retained in the memoization dictionary (while still returning a correctly sanitized/truncated event name).

Changes:

  • Add MaxCachedCategoryNameLength (512) and skip cache insertion for category names longer than this limit.
  • Add unit tests covering the “at limit cached” vs “over limit not cached” behavior for sanitized category names.
  • Update the Geneva exporter changelog entry to reference this follow-up PR alongside the original optimization PR (#4498).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
test/OpenTelemetry.Exporter.Geneva.Tests/TldLogCommonSanitizationTests.cs Adds tests validating caching behavior at and above the maximum cached category-name length.
src/OpenTelemetry.Exporter.Geneva/Internal/Tld/TldLogCommon.cs Introduces a max length guard to avoid caching pathologically long logger category names.
src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md Updates the existing allocations-reduction changelog entry to include this PR reference.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.33%. Comparing base (10c296b) to head (e43cc67).
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4684   +/-   ##
=======================================
  Coverage   77.33%   77.33%           
=======================================
  Files         466      466           
  Lines       19705    19707    +2     
=======================================
+ Hits        15239    15241    +2     
  Misses       4466     4466           
Flag Coverage Δ
unittests-Exporter.Geneva 56.76% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...metry.Exporter.Geneva/Internal/Tld/TldLogCommon.cs 66.91% <100.00%> (+0.50%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@martincostello
martincostello enabled auto-merge July 9, 2026 07:18
@Kielek

Kielek commented Jul 9, 2026

Copy link
Copy Markdown
Member

@petrroll FYI

@Kielek Kielek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@martincostello
martincostello added this pull request to the merge queue Jul 9, 2026
Merged via the queue into open-telemetry:main with commit fc2ac12 Jul 9, 2026
81 checks passed
@martincostello
martincostello deleted the geneva-fix-cache-retention branch July 9, 2026 07:36
This was referenced Aug 31, 2026
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.

3 participants