Skip to content

[OneCollector] Validate full event names - #4857

Open
martincostello wants to merge 6 commits into
open-telemetry:mainfrom
martincostello:onecollector-validate-eventname
Open

[OneCollector] Validate full event names#4857
martincostello wants to merge 6 commits into
open-telemetry:mainfrom
martincostello:onecollector-validate-eventname

Conversation

@martincostello

Copy link
Copy Markdown
Member

Changes

Validate {EventFullName} before serializing to avoid payload corruption.

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)

Validate `{EventFullName}` before serializing to avoid payload corruption.
Comment thread src/OpenTelemetry.Exporter.OneCollector/CHANGELOG.md Outdated
@github-actions github-actions Bot added the comp:exporter.onecollector Things related to OpenTelemetry.Exporter.OneCollector label Jul 21, 2026
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.37%. Comparing base (57c3c2b) to head (d3cb108).
⚠️ Report is 26 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...Exporter.OneCollector/Internal/EventNameManager.cs 95.55% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4857   +/-   ##
=======================================
  Coverage   77.37%   77.37%           
=======================================
  Files         468      468           
  Lines       19812    19831   +19     
=======================================
+ Hits        15329    15345   +16     
- Misses       4483     4486    +3     
Flag Coverage Δ
unittests-Exporter.OneCollector 95.44% <95.74%> (-0.17%) ⬇️

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

Files with missing lines Coverage Δ
...rialization/LogRecordCommonSchemaJsonSerializer.cs 99.25% <100.00%> (-0.02%) ⬇️
...Exporter.OneCollector/Internal/EventNameManager.cs 98.18% <95.55%> (-1.82%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 21, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-08-14 16:20 UTC

Review the latest changes.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@martincostello
martincostello marked this pull request as ready for review July 21, 2026 15:51
@martincostello
martincostello requested a review from a team as a code owner July 21, 2026 15:51
Copilot AI review requested due to automatic review settings July 21, 2026 15:51

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

Adds validation for {EventFullName} values supplied via log attributes in the OneCollector exporter to prevent unsafe/invalid raw JSON from being emitted (which could corrupt payloads). This fits into the exporter’s existing event name management and common-schema JSON serialization pipeline.

Changes:

  • Add IsEventFullNameValid and enforce character + length validation in EventNameManager.ResolveEventFullName(string), falling back to the default event full name when invalid.
  • Add tests to ensure JSON injection is not possible via {EventFullName} and to validate allowed/rejected full-name inputs.
  • Update OneCollector exporter changelog to document the behavior change.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/OpenTelemetry.Exporter.OneCollector/Internal/EventNameManager.cs Validates {EventFullName} inputs (chars/length) before producing raw JSON bytes; logs and falls back to default on invalid input.
test/OpenTelemetry.Exporter.OneCollector.Tests/LogRecordCommonSchemaJsonSerializerTests.cs Adds regression test asserting {EventFullName} cannot inject extra JSON properties into the serialized payload.
test/OpenTelemetry.Exporter.OneCollector.Tests/EventNameManagerTests.cs Adds tests for single-argument {EventFullName} resolution, including unsafe character rejection and length handling.
src/OpenTelemetry.Exporter.OneCollector/CHANGELOG.md Documents the new validation/fallback behavior in ## Unreleased.

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

Comment thread src/OpenTelemetry.Exporter.OneCollector/Internal/EventNameManager.cs Outdated
- Do not cache names that are too long.
- Extend test coverage.
@martincostello
martincostello enabled auto-merge July 21, 2026 16:18
@martincostello martincostello added the keep-open Prevents issues and pull requests being closed as stale label Jul 22, 2026
@rajkumar-rangaraj

Copy link
Copy Markdown
Member

How much performance impact does adding the IsEventFullNameValid check introduce?
Is there a customer ask for this check?

@martincostello

Copy link
Copy Markdown
Member Author

How much performance impact does adding the IsEventFullNameValid check introduce?

I haven't measured it, but I would have thought it minimal as it's just scanning the chars in the name.

Is there a customer ask for this check?

No, I was doing a code audit with Copilot/Claude (I forget which specifically), and it flagged that the validation didn't run in all cases.

@martincostello

Copy link
Copy Markdown
Member Author

No, I was doing a code audit with Copilot/Claude (I forget which specifically), and it flagged that the validation didn't run in all cases.

It was Copilot, and it addresses a path where JSON injection can occur (see the tests).

- Bound the cache of event names.
- Ensure event names and namespaces are valid JSON.
@martincostello

Copy link
Copy Markdown
Member Author

Added some additional hardening based on a scan of the code by Copilot and Claude.

Escape non-ASCII characters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:exporter.onecollector Things related to OpenTelemetry.Exporter.OneCollector keep-open Prevents issues and pull requests being closed as stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants