refactor trace exporter errors to use template pattern #1218
Conversation
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
BenchmarksComparisonBenchmark execution time: 2025-09-15 19:44:43 Comparing candidate commit 20c9bb8 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 51 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/ 378282246310005
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
Group 14
Group 15
BaselineOmitted due to size. |
ekump
force-pushed
the
ekump/template-trace-exporter-errors
branch
from
b845566 to
f027fb4
Compare
ekump
force-pushed
the
ekump/template-trace-exporter-errors
branch
from
f027fb4 to
b39e712
Compare
ekump
commented
Sep 12, 2025
| /// Contains key-value pairs that can be safely used for logging or debugging. | ||
| #[derive(Debug, Clone, Default, PartialEq)] | ||
| pub struct ErrorContext { | ||
| fields: Vec<(String, String)>, |
There was a problem hiding this comment.
Went with this over a HashMap because it should be more efficient when there are only a few entries. Also, I can't imagine when we'd need to do a key lookup so why incur the hashing costs?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
+ Coverage 71.55% 71.72% +0.17%
==========================================
Files 354 354
Lines 55959 56438 +479
==========================================
+ Hits 40042 40481 +439
- Misses 15917 15957 +40
🚀 New features to boost your workflow:
|
ekump
force-pushed
the
ekump/template-trace-exporter-errors
branch
from
b39e712 to
045d040
Compare
ganeshnj
previously requested changes
Sep 17, 2025
Comment on lines
+14
to
+16
| pub key: *const c_char, | ||
| /// Value for this context field | ||
| pub value: *const c_char, |
There was a problem hiding this comment.
for consistency, we should use CharSlice.
There was a problem hiding this comment.
Suggested change
| pub key: *const c_char, | |
| /// Value for this context field | |
| pub value: *const c_char, | |
| pub key: CharSlice, | |
| /// Value for this context field | |
| pub value: CharSlice, |
Comment on lines
+90
to
+92
| pub context_fields: *const ContextField, | ||
| /// Number of context fields | ||
| pub context_count: usize, |
There was a problem hiding this comment.
Did you try using Vec https://github.com/DataDog/libdatadog/blob/a156b6a8daebe95a006290868ace32fb78625d72/ddcommon-ffi/src/vec.rs
we already have it integrated in the tracersm and don't have to reinvent.
| pub code: ExporterErrorCode, | ||
| pub msg: *mut c_char, | ||
| /// Static error message template | ||
| pub msg_template: *const c_char, |
There was a problem hiding this comment.
would there be different msg_template for same error code? if not - this seems redundant to me.
| unsafe { | ||
| // Free the msg_template | ||
| if !self.msg_template.is_null() { | ||
| // SAFETY: msg_template is originated from CString::into_raw in new() and |
There was a problem hiding this comment.
by definition template is static string, so ideally this is a no-op.
can it be avoided?
There was a problem hiding this comment.
It's not it is a CString created from a static string. Although I wonder if could use a const CString instead of creating a CString from the 'static str to skip allocations.
| } | ||
|
|
||
| impl BuilderErrorKind { | ||
| const INVALID_URI_TEMPLATE: &'static str = "Invalid URI provided: {details}"; |
There was a problem hiding this comment.
how {details} will be interpreted by the tracers?
imo we should not have any templates in the template message just like structured logging.
- error code
- error message (without any template)
- context (key-value collection)
| void handle_error(ddog_TraceExporterError *err) { | ||
| fprintf(stderr, "Operation failed with error: %d, reason: %s\n", err->code, err->msg); | ||
| fprintf(stderr, "Operation failed with error: %d\n", err->code); | ||
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
TraceExporterErroris now template based. The error message is static and dynamic context is included separately as key value pairs. In Rust, theDisplaytrait can be used to get a string that combines the template and context for standard error logging scenarios. For cases where we want to ensure we don't include any dynamic content the template field can be used. The FFI layer has also been updated to separate template and context.Motivation
Sometimes we don't want to log strings with dynamic content in them.
Additional Notes
Breaking change for FFI API users
For ExporterError
msg: *mut c_char*has been replaced withmsg_template: *const c_char. New fields have also been added.Possible Breaking change for Rust API users
The public API changes to
TraceExporterErrorare additive, but the format of the string returned by theDisplaytrait has changed.How to test the change?
Describe here in detail how the change can be validated.