feat: add SQLCommenter custom fields API with thread-local context #1
Conversation
Implements a thread-local context system for injecting custom key-value pairs into SQL comments, similar to SLF4J MDC pattern. This allows external users to add custom metadata to SQL traces. Features: - Thread-local context management for custom fields - Scoped execution with withSQLCommentFields() pattern - Public API in dd-trace-api with reflection-based access - Proper URL encoding for SQL comment safety - Comprehensive test coverage for all functionality - Maintains backward compatibility with existing SQLCommenter The implementation includes: - SQLCommenterContext: Thread-local context system - SQLCommenter API: Public interface for external users - Integration with existing SQL comment injection - Exception handling with context restoration - Thread isolation and nested context support
|
|
||
| /** | ||
| * Executes a block of code with the given context map, restoring the original context afterward. | ||
| * This method is similar to the withLoggingFields pattern from the provided Kotlin code. |
There was a problem hiding this comment.
Oh thanks, I worked with Claude to help generate parts of this and I remembered it spat this comment out but forgot where / to remove it.
I fed in the example from https://github.com/mdg-private/monorepo/blob/main/mdg/common/src/main/kotlin/mdg/common/MDC.kt to level-set on the pattern I was describing.
There was a problem hiding this comment.
yup, had a feeling I knew where that came from. Ideally I know our goal is to try this out internally, and then get DD to accept it, so was trying to be really thorough for you. Everything else looked 👍
There was a problem hiding this comment.
Thank you, very much appreciated!
glasser
left a comment
glasser
left a comment
There was a problem hiding this comment.
looks pretty good on the whole! i think we should keep the API here tiny/low-level.
| if (customFields != null) { | ||
| for (Map.Entry<String, String> entry : customFields.entrySet()) { | ||
| if (entry.getKey() != null && entry.getValue() != null) { | ||
| // Account for URL encoding overhead (approximately 3x for worst case) |
There was a problem hiding this comment.
It doesn't seem like the existing implementation does this for values? And it does do it for keys implicity (by calling length on the encoded version) but in practice the keys are unlikely to need much escaping... I'd ditch the multiplications? (This is just initial allocation length so there' sno correctness issue with messing this up.)
| } | ||
| } | ||
|
|
||
| // Direct access to the context - simpler than the provider pattern |
There was a problem hiding this comment.
Hopefully this ends up being the same class from the same classloader? I guess if it works it works.
| * @param key the key | ||
| * @param value the value (will be converted to string) | ||
| */ | ||
| public static void put(String key, Object value) { |
There was a problem hiding this comment.
I think it might be better to keep this patch a lot simpler and just have getCopyOfContextMap/setContextMap rather than the put/get/with APIs. Notably, to use this with Kotlin coroutine suspending functions we'll have to do something more like withLoggingFieldsSuspend which does some special stuff with Kotlin coroutine withContext and ThreadContextElement to basically implement its own version of keeping things in the context when coroutines switch off and on threads — ie, that's the only primitives we'll actually use.
Implements a thread-local context system for injecting custom key-value pairs into SQL comments, similar to SLF4J MDC pattern. This allows external users to add custom metadata to SQL traces.
Features:
The implementation includes:
What Does This Do
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]