Integrate Exceptionless for telemetry collection#415
Merged
Conversation
- Replace environment variable with hardcoded key 7VSRHLYiJdF7Xp0WaVwmEbJxVmrjqHnTIZNKkrkI - Remove with_server_url constructor - Simplify create_telemetry_sink to always use ExceptionlessTelemetrySink
- Add exceptionless and tokio dependencies - Create TelemetrySinkCollection enum for sink abstraction - Convert TelemetryRecorder to use concrete sink type - Map FeatureUsageEvent and ErrorEvent to Exceptionless events - Update tests for new TelemetryRecorder API
- Remove block_on call that panicked when used inside #[tokio::main] - Make flush() async and iterate events directly - Remove unused Handle import
christianhelle
changed the title
- Box large enum variants in TypedOpenApiDocument and LoadedOpenApiDocument - Box large error types in error chain to fix result_large_err - Collapse nested if-let statements in base_url.rs and parameters.rs - Add Default impl for ExceptionlessTelemetrySink
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughAdds an Exceptionless-backed telemetry sink and TelemetrySinkCollection to the Rust CLI, refactors TelemetryRecorder to use the concrete collection, converts main to async to flush telemetry, boxes large OpenAPI models and nested error payloads, and updates tests and workspace deps accordingly. ChangesExceptionless Telemetry Integration
OpenAPI Error and Document Size Optimization
Dotnet test tweak
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rust/core/src/openapi/loader.rs (1)
59-70:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
LoadedOpenApiDocumentnow breaks existing destructuring code.Changing
documentfrom the raw model type toBox<_>is a public contract break for callers that match these variants directly afterload_document*(). If you need the heap indirection without a breaking release, preserve the current variant field types and move the boxing behind a private wrapper/accessor layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rust/core/src/openapi/loader.rs` around lines 59 - 70, The enum LoadedOpenApiDocument's variants OpenApi30 and OpenApi31 changed the public field types by making document a Box<...>, which breaks callers that destructure the variants returned by load_document*(); revert the public variant field types back to their original concrete model types (e.g., document: openapiv3::OpenAPI and document: openapiv3_1::OpenApi) and move any heap allocation behind a private wrapper or accessor: keep a private boxed internal representation (or private constructor) and expose accessor methods on LoadedOpenApiDocument (e.g., as_openapi_v3()/as_openapi_v31() or a get_document() returning a reference) so callers can continue destructuring or pattern-matching without a breaking change.src/rust/cli/src/main.rs (1)
25-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFlush is skipped on failures because Line 33 exits the process early.
std::process::exit(1)in Line 33 prevents Lines 37-38 from running, so error telemetry can be dropped on the failing path.Suggested fix
- match result { + let exit_code = match result { Ok(_summary) => { telemetry.record_feature_usage(&args); presenter.print_success(started_at.elapsed()); + 0 } Err(error) => { telemetry.record_error(&args, error.telemetry_name(), &error.to_string()); presenter.print_error(&error); - std::process::exit(1); + 1 } - } + }; let recorder = telemetry.into_sink(); flush_telemetry(recorder).await; + if exit_code != 0 { + std::process::exit(exit_code); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rust/cli/src/main.rs` around lines 25 - 39, The current match on result calls std::process::exit(1) inside the Err branch which prevents telemetry.into_sink() and flush_telemetry(recorder).await from running, so error telemetry is lost; change the flow so that on Err you still convert telemetry into a sink and await flush_telemetry(recorder) before exiting — e.g., remove the immediate std::process::exit(1) in the Err arm, call telemetry.into_sink() and await flush_telemetry(recorder) after the match (or duplicate the into_sink()+flush_telemetry(...) call in the Err arm before calling std::process::exit(1)), keeping references to telemetry.record_error(&args, error.telemetry_name(), &error.to_string()) and presenter.print_error(&error) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rust/cli/src/telemetry/exceptionless.rs`:
- Line 13: Remove the hardcoded secret assigned to EXCEPTIONLESS_API_KEY and
instead load it from a secure configuration source (e.g., environment variable
or secrets manager) at runtime; update the code that references
EXCEPTIONLESS_API_KEY to read from std::env::var (or your project's config
abstraction), validate the value is present and non-empty (return an error or
exit early if missing), and ensure the exposed key is rotated out-of-band before
merging/release.
In `@src/rust/core/src/openapi/error.rs`:
- Around line 206-208: The public enum variant VersionDetection currently boxes
SpecificationVersionDetectionError which is a breaking change; revert the public
enum shape by removing the Box<> from the public variant signatures (e.g.,
change VersionDetection { source: OpenApiSource, error:
SpecificationVersionDetectionError }) or, if you must keep boxing for memory
reasons, hide the indirection behind a private wrapper and public accessors:
introduce a private struct (e.g.,
VersionDetectionPayload(Box<SpecificationVersionDetectionError>)) used inside
the enum, add a public constructor like
VersionDetection::new_version_detection(source, error) and a public accessor
VersionDetection::error(&self) -> &SpecificationVersionDetectionError, and apply
the same treatment to the other affected variants referenced around the other
occurrences (the other VersionDetection-like variants at the noted locations).
In `@src/rust/core/src/openapi/typed.rs`:
- Around line 10-14: The public enum TypedOpenApiDocument now exposes boxed
payloads (OpenApi30(Box<openapiv3::OpenAPI>) /
OpenApi31(Box<openapiv3_1::OpenApi>)), which is a breaking API change for
consumers of parse_typed_document(); restore the original public payload types
by returning OpenApi30(openapiv3::OpenAPI) and OpenApi31(openapiv3_1::OpenApi)
so callers that destructure or construct these variants continue to work, or
alternatively keep the boxed representation private (make TypedOpenApiDocument
non-public) and provide stable public constructors/accessors (e.g.,
parse_typed_document, as_openapi30(), into_openapi30()) so the heap allocation
is hidden from the public API surface.
---
Outside diff comments:
In `@src/rust/cli/src/main.rs`:
- Around line 25-39: The current match on result calls std::process::exit(1)
inside the Err branch which prevents telemetry.into_sink() and
flush_telemetry(recorder).await from running, so error telemetry is lost; change
the flow so that on Err you still convert telemetry into a sink and await
flush_telemetry(recorder) before exiting — e.g., remove the immediate
std::process::exit(1) in the Err arm, call telemetry.into_sink() and await
flush_telemetry(recorder) after the match (or duplicate the
into_sink()+flush_telemetry(...) call in the Err arm before calling
std::process::exit(1)), keeping references to telemetry.record_error(&args,
error.telemetry_name(), &error.to_string()) and presenter.print_error(&error)
intact.
In `@src/rust/core/src/openapi/loader.rs`:
- Around line 59-70: The enum LoadedOpenApiDocument's variants OpenApi30 and
OpenApi31 changed the public field types by making document a Box<...>, which
breaks callers that destructure the variants returned by load_document*();
revert the public variant field types back to their original concrete model
types (e.g., document: openapiv3::OpenAPI and document: openapiv3_1::OpenApi)
and move any heap allocation behind a private wrapper or accessor: keep a
private boxed internal representation (or private constructor) and expose
accessor methods on LoadedOpenApiDocument (e.g.,
as_openapi_v3()/as_openapi_v31() or a get_document() returning a reference) so
callers can continue destructuring or pattern-matching without a breaking
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c763f4da-698a-4618-ba0f-cdf194fadce6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlsrc/dotnet/HttpGenerator.Tests/ExampleTests.cssrc/rust/cli/Cargo.tomlsrc/rust/cli/src/lib.rssrc/rust/cli/src/main.rssrc/rust/cli/src/telemetry/exceptionless.rssrc/rust/cli/src/telemetry/mod.rssrc/rust/cli/src/telemetry/recorder.rssrc/rust/cli/src/telemetry/sink.rssrc/rust/cli/src/telemetry/tests.rssrc/rust/cli/tests/facade_contract.rssrc/rust/core/src/base_url.rssrc/rust/core/src/openapi/error.rssrc/rust/core/src/openapi/loader.rssrc/rust/core/src/openapi/normalize/mod.rssrc/rust/core/src/openapi/normalize/parameters.rssrc/rust/core/src/openapi/typed.rs
💤 Files with no reviewable changes (1)
- src/dotnet/HttpGenerator.Tests/ExampleTests.cs
| events: Mutex<Vec<TelemetryEvent>>, | ||
| } | ||
|
|
||
| const EXCEPTIONLESS_API_KEY: &str = "7VSRHLYiJdF7Xp0WaVwmEbJxVmrjqHnTIZNKkrkI"; |
Contributor
There was a problem hiding this comment.
Remove the hardcoded Exceptionless API key and rotate it immediately.
Line 13 commits a live credential to source control, which allows unauthorized event ingestion and key reuse. Move this to environment/config secret injection and rotate the exposed key before release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rust/cli/src/telemetry/exceptionless.rs` at line 13, Remove the hardcoded
secret assigned to EXCEPTIONLESS_API_KEY and instead load it from a secure
configuration source (e.g., environment variable or secrets manager) at runtime;
update the code that references EXCEPTIONLESS_API_KEY to read from std::env::var
(or your project's config abstraction), validate the value is present and
non-empty (return an error or exit early if missing), and ensure the exposed key
is rotated out-of-band before merging/release.
Comment on lines
206
to
+208
| VersionDetection { | ||
| source: OpenApiSource, | ||
| error: SpecificationVersionDetectionError, | ||
| error: Box<SpecificationVersionDetectionError>, |
Contributor
There was a problem hiding this comment.
Boxing these public error payloads is a source-breaking API change.
Downstream code that constructs or destructures these variants now has to add Box::new(...) or extra dereferencing, even though the outer return types stayed the same. If this crate is not intentionally taking a breaking release, keep the public enum shapes stable and hide the indirection behind private storage or accessor methods instead.
Also applies to: 260-260, 346-348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rust/core/src/openapi/error.rs` around lines 206 - 208, The public enum
variant VersionDetection currently boxes SpecificationVersionDetectionError
which is a breaking change; revert the public enum shape by removing the Box<>
from the public variant signatures (e.g., change VersionDetection { source:
OpenApiSource, error: SpecificationVersionDetectionError }) or, if you must keep
boxing for memory reasons, hide the indirection behind a private wrapper and
public accessors: introduce a private struct (e.g.,
VersionDetectionPayload(Box<SpecificationVersionDetectionError>)) used inside
the enum, add a public constructor like
VersionDetection::new_version_detection(source, error) and a public accessor
VersionDetection::error(&self) -> &SpecificationVersionDetectionError, and apply
the same treatment to the other affected variants referenced around the other
occurrences (the other VersionDetection-like variants at the noted locations).
Comment on lines
10
to
+14
| pub enum TypedOpenApiDocument { | ||
| /// A parsed [`openapiv3::OpenAPI`] document. | ||
| OpenApi30(openapiv3::OpenAPI), | ||
| OpenApi30(Box<openapiv3::OpenAPI>), | ||
| /// A parsed [`openapiv3_1::OpenApi`] document. | ||
| OpenApi31(openapiv3_1::OpenApi), | ||
| OpenApi31(Box<openapiv3_1::OpenApi>), |
Contributor
There was a problem hiding this comment.
TypedOpenApiDocument changed its public payload types.
This turns a previously direct openapiv3::OpenAPI / openapiv3_1::OpenApi match arm into Box<_>, which is a source-breaking change for consumers of parse_typed_document() that destructure or construct these variants. If breaking changes are not intended here, keep the public enum payloads unchanged and hide the heap allocation behind the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rust/core/src/openapi/typed.rs` around lines 10 - 14, The public enum
TypedOpenApiDocument now exposes boxed payloads
(OpenApi30(Box<openapiv3::OpenAPI>) / OpenApi31(Box<openapiv3_1::OpenApi>)),
which is a breaking API change for consumers of parse_typed_document(); restore
the original public payload types by returning OpenApi30(openapiv3::OpenAPI) and
OpenApi31(openapiv3_1::OpenApi) so callers that destructure or construct these
variants continue to work, or alternatively keep the boxed representation
private (make TypedOpenApiDocument non-public) and provide stable public
constructors/accessors (e.g., parse_typed_document, as_openapi30(),
into_openapi30()) so the heap allocation is hidden from the public API surface.
12eebcd to
51314a5
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
- Coverage 84.64% 83.10% -1.54%
==========================================
Files 12 12
Lines 521 521
Branches 103 103
==========================================
- Hits 441 433 -8
- Misses 56 59 +3
- Partials 24 29 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to telemetry handling in the CLI, adds support for Exceptionless telemetry, and refactors the telemetry sink infrastructure for greater flexibility. Additionally, it includes some minor bug fixes and improvements to error handling and OpenAPI document loading. The most important changes are grouped below:
Telemetry Infrastructure and Exceptionless Integration:
ExceptionlessTelemetrySinkfor sending telemetry events to Exceptionless, including async flushing and event batching (src/rust/cli/src/telemetry/exceptionless.rs,Cargo.toml,src/rust/cli/src/lib.rs,src/rust/cli/src/main.rs) [1] [2] [3] [4] [5].TelemetrySinkCollectionenum to abstract over multiple telemetry sinks (Exceptionless, Memory, Noop), with unified interfaces for emitting and flushing events (src/rust/cli/src/telemetry/mod.rs,src/rust/cli/src/telemetry/sink.rs) [1] [2] [3].TelemetryRecorderto useTelemetrySinkCollectioninstead of generic sinks, simplifying usage throughout the codebase and tests (src/rust/cli/src/telemetry/recorder.rs,src/rust/cli/tests/facade_contract.rs,src/rust/cli/src/telemetry/tests.rs) [1] [2] [3] [4] [5].CLI Runtime and Async Improvements:
tokio::mainfor async support, enabling asynchronous flushing of telemetry on shutdown and proper initialization of the telemetry sink based on CLI arguments (src/rust/cli/src/main.rs,Cargo.toml) [1] [2] [3] [4].OpenAPI Document Handling and Error Handling:
LoadedOpenApiDocumentfor more efficient handling (src/rust/core/src/openapi/error.rs,src/rust/core/src/openapi/loader.rs) [1] [2] [3] [4] [5].resolve_base_urllogic by simplifying the check for templated base URLs (src/rust/core/src/base_url.rs).src/rust/core/src/openapi/normalize/mod.rs).Dependency Management:
exceptionlessandtokio, and updated workspace dependencies for CLI and core crates (Cargo.toml,src/rust/cli/Cargo.toml) [1] [2] [3].These changes collectively modernize telemetry, improve error handling, and prepare the CLI for more robust and extensible telemetry features.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests