refactor: internal OTLP types + remove ghost code - #2
Conversation
Encapsulate OTLP implementation details within qyl.collector: - OtlpGenAiAttributes, SchemaNormalizer → internal - All OTLP DTOs (OtlpSpan, OtlpResource, etc.) → internal - OtlpConverter, OtlpJsonSpanParser → internal - QylSerializerContext → internal - Well-known value classes → internal Add InternalsVisibleTo for qyl.collector.tests to maintain test access. Update test to use int instead of SpanKind enum in InlineData attributes. This enforces clean architecture boundaries - only qyl.protocol exposes public APIs for external consumption. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Delete obsolete generated files from core/collector/ and core/protocol/. These files are duplicates - the generator now writes directly to src/. Removed: - core/collector/Storage/DuckDbSchema.g.cs - core/protocol/Enums/Enums.g.cs - core/protocol/Models/Types.g.cs - core/protocol/Primitives/Scalars.g.cs The authoritative generated files remain in: - src/qyl.collector/Storage/DuckDbSchema.g.cs - src/qyl.protocol/*/ Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThis change removes four auto-generated files containing public types, enums, and scalars (DuckDB schema, protocol enums, data models, and primitive scalars), while converting remaining OTLP-related types and serialization classes from public to internal visibility. A test project is granted internal type access via InternalsVisibleTo declaration, and a test method signature is updated to accommodate the visibility changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ANcpLua, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the internal architecture and maintainability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request successfully refactors internal OTLP types to internal visibility and removes obsolete generated files, aligning with the goal of enforcing clean architecture boundaries. The changes to the test project (qyl.collector.csproj and OtlpJsonSpanParserTests.cs) correctly adapt to the visibility changes, ensuring tests continue to function as expected. This is a positive change for code organization and maintainability.
There was a problem hiding this comment.
Pull request overview
This PR refactors the OTLP ingestion layer to enforce clean architecture boundaries by making all OTLP implementation types internal to qyl.collector, and removes obsolete auto-generated files from the core/ directory that are now replaced by files in src/qyl.protocol/.
Changes:
- Changed OTLP DTOs, parsers, converters, and enums from public to internal visibility
- Added
InternalsVisibleTofor test project to access internal types - Removed obsolete auto-generated files from
core/protocol/(moved tosrc/qyl.protocol/) - Updated test to use integer comparisons instead of enum direct comparisons for SpanKind
- Improved code comments in project file for clarity
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/qyl.collector.tests/Ingestion/OtlpJsonSpanParserTests.cs | Updated test to use integer-based comparison for SpanKind values instead of direct enum references |
| src/qyl.collector/qyl.collector.csproj | Added InternalsVisibleTo for test project, improved comments for NoWarn suppressions, removed unused VSTHRD111 suppression |
| src/qyl.collector/Program.cs | Changed QylSerializerContext from public to internal |
| src/qyl.collector/Ingestion/OtlpJsonSpanParser.cs | Changed from public to internal ref struct |
| src/qyl.collector/Ingestion/OtlpConverter.cs | Changed from public to internal static class |
| src/qyl.collector/Ingestion/OtlpAttributes.cs | Changed all OTLP-related classes and enums from public to internal (8 classes/enums total) |
| core/protocol/Primitives/Scalars.g.cs | Deleted obsolete generated file (now in src/qyl.protocol/) |
| core/protocol/Models/Types.g.cs | Deleted obsolete generated file (now in src/qyl.protocol/) |
| core/protocol/Enums/Enums.g.cs | Deleted obsolete generated file (now in src/qyl.protocol/) |
| core/collector/Storage/DuckDbSchema.g.cs | Deleted obsolete generated file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Summary
internalvisibilitycore/directory (ghost code)InternalsVisibleTofor test projectMotivation
Enforces clean architecture boundaries - only
qyl.protocolexposes public APIs. The OTLP ingestion layer is an implementation detail of qyl.collector.Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.