CASSJAVA-97: Let users inject an ID for each request and write to the custom payload #2037
Conversation
|
I did integration testing with C* OSS 5.0.2. @lukasz-antoniak helped me add a Running this client app, I got 17:03:20.860 [s0-io-5] TRACE InFlightHandler - [s0|id: 0xeefaab65, L:/127.0.0.2:52389 - R:/127.0.0.2:9042] Writing 00-d51ed2012c1f31b4434f409e50294da9-25da248d0a23981c-00 on stream id 0
17:03:20.863 [s0-io-5] TRACE CqlRequestHandler$NodeResponseCallback - [00-d51ed2012c1f31b4434f409e50294da9-25da248d0a23981c-00] Request sent on [id: 0xeefaab65, L:/127.0.0.2:52389 - R:/127.0.0.2:9042]
17:03:20.864 [s0-io-5] TRACE CqlRequestHandler$NodeResponseCallback - [00-d51ed2012c1f31b4434f409e50294da9-25da248d0a23981c-00] Speculative execution policy returned -1, no next execution
17:03:20.877 [s0-io-5] DEBUG InFlightHandler - [s0|id: 0xeefaab65, L:/127.0.0.2:52389 - R:/127.0.0.2:9042] Got last response on in-flight stream id 0, completing and releasing
17:03:20.877 [s0-io-5] TRACE InFlightHandler - [s0|id: 0xeefaab65, L:/127.0.0.2:52389 - R:/127.0.0.2:9042] Releasing stream id 0
17:03:20.877 [s0-io-5] TRACE CqlRequestHandler$NodeResponseCallback - [00-d51ed2012c1f31b4434f409e50294da9-25da248d0a23981c-00] Got result, completingAnd the
The value |
...ava/com/datastax/oss/driver/internal/core/tracker/W3CContextDistributedTraceIdGenerator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java
Outdated
Show resolved
Hide resolved
| // We cannot do statement.getCustomPayload().put() because the default empty map is abstract | ||
| // But this will create new Statement instance for every request. We might want to optimize | ||
| // this | ||
| Map<String, ByteBuffer> existingMap = new HashMap<>(statement.getCustomPayload()); |
There was a problem hiding this comment.
Statement is by design immutable. Maybe a nicer way would be to create method StatementBuilder.from(Statement) where you could create builder again based on statement. The code would look like: StatementBuilder.from(statement).addCustomPayload(...).build().
There was a problem hiding this comment.
I think you can copy just the payload, not the whole statement:
Map<String, ByteBuffer> customPayload = statement.getCustomPayload();
if (!this.customPayloadKey.isEmpty()) {
customPayload =
NullAllowingImmutableMap.<String, ByteBuffer>builder()
.putAll(customPayload)
.put(
this.customPayloadKey,
ByteBuffer.wrap(nodeRequestId.getBytes(StandardCharsets.UTF_8)))
.build();
}Then modify line 307 like so:
channel
- .write(message, statement.isTracing(), statement.getCustomPayload(), nodeResponseCallback)
+ .write(message, statement.isTracing(), customPayload, nodeResponseCallback)
.addListener(nodeResponseCallback);There was a problem hiding this comment.
This solves the concurrency problem, but it also means the subsequent setFinalError(statement...), NodeResponseCallback(statement,...), and RequestTracker invocations do not have the statement with the actual custom payload.
| Map<String, ByteBuffer> existingMap = new HashMap<>(statement.getCustomPayload()); | ||
| existingMap.put( | ||
| this.customPayloadKey, ByteBuffer.wrap(nodeRequestId.getBytes(StandardCharsets.UTF_8))); | ||
| statement = statement.setCustomPayload(existingMap); |
There was a problem hiding this comment.
Overriding custom payload here is not thread-safe. If client application executes the same statement instance multiple times concurrently (not a good use-case, but still possible), we do not guarantee how this map will be changed. Maybe indeed, there is no other way than make a shallow copy of the statement. Will think about it.
/**
* Sets the custom payload to use for execution.
*
* <p>All the driver's built-in statement implementations are immutable, and return a new instance
* from this method. However custom implementations may choose to be mutable and return the same
* instance.
*
* <p>Note that it's your responsibility to provide a thread-safe map. This can be achieved with a
* concurrent or immutable implementation, or by making it effectively immutable (meaning that
* it's never modified after being set on the statement).
*/
@NonNull
@CheckReturnValue
SelfT setCustomPayload(@NonNull Map<String, ByteBuffer> newCustomPayload);
| * Registers a distributed trace ID generator The driver will use the distributed trace ID in the | ||
| * logs So that users can correlate logs about the same request from different loggers. |
There was a problem hiding this comment.
nitpick: it looks like a period is missing here.
| * Registers a distributed trace ID generator The driver will use the distributed trace ID in the | |
| * logs So that users can correlate logs about the same request from different loggers. | |
| * Registers a distributed trace ID generator. The driver will use the distributed trace ID in the | |
| * logs So that users can correlate logs about the same request from different loggers. |
Also if there's nothing useful to say in @param and @return (and I would agree it is the case here), it's fine to omit them altogether.
| @Override | ||
| public String getNodeRequestId( | ||
| @NonNull Request statement, @NonNull String sessionRequestId, int executionCount) { | ||
| return sessionRequestId + "-" + Uuids.random().toString(); |
There was a problem hiding this comment.
nitpick: the .toString() at the end is unnecessary.
| import com.datastax.oss.driver.api.core.session.Request; | ||
| import edu.umd.cs.findbugs.annotations.NonNull; | ||
|
|
||
| public interface DistributedTraceIdGenerator { |
There was a problem hiding this comment.
nitpick: this is subjective, but the name is a bit long. Have you considered just TraceIdGenerator?
There was a problem hiding this comment.
The name trace ID may be confused with the QueryTrace feature.
There was a problem hiding this comment.
But I agree, I think this name is a bit too long, too.
|
|
||
| public interface DistributedTraceIdGenerator { | ||
| /** | ||
| * Generates a unique identifier for the session request. This identifier will be added to logs. |
There was a problem hiding this comment.
I think we should be more descriptive here, it was not obvious to me on the first read .
Something along the lines of:
- sessionRequestId: an identifier for an entire
session.execute()call - nodeRequestId: an identifier for the execution of a statement against a particular node. There can be multiple nodeRequestId for a given sessionRequestId (because of retries and speculative executions). Can optionally be sent to the node in the custom payload.
Both are propagated to request trackers (btw, we should rename the corresponding parameters in RequestTracker and adjust the descriptions).
absurdfarce
changed the title
…ution info does not have actual custom payload
No description provided.