Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Commit

Permalink
Refactor TracingKVS (#2643)
Browse files Browse the repository at this point in the history 
* Wrap next() and hasNext() in traces

* Use span names as safe

* Remove iterator wrappings

* checkstyle

* refactor methods and remove misleading traces

* Fix unit tests

* release notes

* Final nits

* fix java arrays usage
  • Loading branch information
@fsamuel-bs
fsamuel-bs authored Nov 8, 2017
1 parent f116645 commit c1e21ee
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 116 deletions.
92 changes: 45 additions & 47 deletions atlasdb-client/src/main/java/com/palantir/atlasdb/keyvalue/impl/TracingKeyValueService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,13 @@
import com.palantir.atlasdb.keyvalue.api.RowResult;
import com.palantir.atlasdb.keyvalue.api.TableReference;
import com.palantir.atlasdb.keyvalue.api.Value;
import com.palantir.atlasdb.logging.LoggingArgs;
import com.palantir.atlasdb.tracing.CloseableTrace;
import com.palantir.common.base.ClosableIterator;
import com.palantir.util.paging.TokenBackedBasicResultsPage;

/**
* Wraps a {@link KeyValueService}'s methods with {@link com.palantir.remoting2.tracing.Tracer}
* Wraps a {@link KeyValueService}'s methods with {@link com.palantir.remoting3.tracing.Tracer}
* instrumentation.
*/
public final class TracingKeyValueService extends ForwardingObject implements KeyValueService {
Expand Down Expand Up @@ -76,16 +77,16 @@ protected KeyValueService delegate() {
public void addGarbageCollectionSentinelValues(TableReference tableRef, Iterable<Cell> cells) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("addGarbageCollectionSentinelValues({}, {} cells)",
tableRef, Iterables.size(cells))) {
LoggingArgs.safeTableOrPlaceholder(tableRef), Iterables.size(cells))) {
delegate().addGarbageCollectionSentinelValues(tableRef, cells);
}
}

@Override
public void checkAndSet(CheckAndSetRequest checkAndSetRequest) throws CheckAndSetException {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("checkAndSet({}, {})",
checkAndSetRequest.table(), checkAndSetRequest.cell())) {
try (CloseableTrace trace = startLocalTrace("checkAndSet({})",
LoggingArgs.safeTableOrPlaceholder(checkAndSetRequest.table()))) {
delegate().checkAndSet(checkAndSetRequest);
}
}
Expand All @@ -101,7 +102,8 @@ public void close() {
@Override
public void compactInternally(TableReference tableRef) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("compactInternally({})", tableRef)) {
try (CloseableTrace trace = startLocalTrace("compactInternally({})",
LoggingArgs.safeTableOrPlaceholder(tableRef))) {
delegate().compactInternally(tableRef);
}
}
Expand All @@ -122,7 +124,8 @@ public boolean isInitialized() {
@Override
public void createTable(TableReference tableRef, byte[] tableMetadata) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("createTable({})", tableRef)) {
try (CloseableTrace trace = startLocalTrace("createTable({})",
LoggingArgs.safeTableOrPlaceholder(tableRef))) {
delegate().createTable(tableRef, tableMetadata);
}
}
Expand All @@ -131,39 +134,43 @@ public void createTable(TableReference tableRef, byte[] tableMetadata) {
public void createTables(Map<TableReference, byte[]> tableNamesToTableMetadata) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("createTables({})",
tableNamesToTableMetadata.keySet())) {
LoggingArgs.safeTablesOrPlaceholder(tableNamesToTableMetadata.keySet()))) {
delegate().createTables(tableNamesToTableMetadata);
}
}

@Override
public void delete(TableReference tableRef, Multimap<Cell, Long> keys) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("delete({}, {} keys)", tableRef, keys.size())) {
try (CloseableTrace trace = startLocalTrace("delete({}, {} keys)",
LoggingArgs.safeTableOrPlaceholder(tableRef), keys.size())) {
delegate().delete(tableRef, keys);
}
}

@Override
public void deleteRange(TableReference tableRef, RangeRequest range) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("deleteRange({})", tableRef)) {
try (CloseableTrace trace = startLocalTrace("deleteRange({})",
LoggingArgs.safeTableOrPlaceholder(tableRef))) {
delegate().deleteRange(tableRef, range);
}
}

@Override
public void dropTable(TableReference tableRef) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("dropTable({})", tableRef)) {
try (CloseableTrace trace = startLocalTrace("dropTable({})",
LoggingArgs.safeTableOrPlaceholder(tableRef))) {
delegate().dropTable(tableRef);
}
}

@Override
public void dropTables(Set<TableReference> tableRefs) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("dropTables({})", tableRefs)) {
try (CloseableTrace trace = startLocalTrace("dropTables({})",
LoggingArgs.safeTablesOrPlaceholder(tableRefs))) {
delegate().dropTables(tableRefs);
}
}
Expand All @@ -172,7 +179,7 @@ public void dropTables(Set<TableReference> tableRefs) {
public Map<Cell, Value> get(TableReference tableRef, Map<Cell, Long> timestampByCell) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("get({}, {} cells)",
tableRef, timestampByCell.size())) {
LoggingArgs.safeTableOrPlaceholder(tableRef), timestampByCell.size())) {
return delegate().get(tableRef, timestampByCell);
}
}
Expand All @@ -189,7 +196,7 @@ public Set<TableReference> getAllTableNames() {
public Multimap<Cell, Long> getAllTimestamps(TableReference tableRef, Set<Cell> keys, long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getAllTimestamps({}, {} keys, ts {})",
tableRef, keys.size(), timestamp)) {
LoggingArgs.safeTableOrPlaceholder(tableRef), keys.size(), timestamp)) {
return delegate().getAllTimestamps(tableRef, keys, timestamp);
}
}
Expand All @@ -209,7 +216,7 @@ public Map<RangeRequest, TokenBackedBasicResultsPage<RowResult<Value>, byte[]>>
long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getFirstBatchForRanges({}, {} ranges, ts {})",
tableRef, Iterables.size(rangeRequests), timestamp)) {
LoggingArgs.safeTableOrPlaceholder(tableRef), Iterables.size(rangeRequests), timestamp)) {
return delegate().getFirstBatchForRanges(tableRef, rangeRequests, timestamp);
}
}
Expand All @@ -219,15 +226,16 @@ public Map<Cell, Long> getLatestTimestamps(TableReference tableRef,
Map<Cell, Long> timestampByCell) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getLatestTimestamps({}, {} cells)",
tableRef, timestampByCell.size())) {
LoggingArgs.safeTableOrPlaceholder(tableRef), timestampByCell.size())) {
return delegate().getLatestTimestamps(tableRef, timestampByCell);
}
}

@Override
public byte[] getMetadataForTable(TableReference tableRef) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getMetadataForTable({})", tableRef)) {
try (CloseableTrace trace = startLocalTrace("getMetadataForTable({})",
LoggingArgs.safeTableOrPlaceholder(tableRef))) {
return delegate().getMetadataForTable(tableRef);
}
}
Expand All @@ -244,32 +252,23 @@ public Map<TableReference, byte[]> getMetadataForTables() {
public ClosableIterator<RowResult<Value>> getRange(TableReference tableRef,
RangeRequest rangeRequest,
long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getRange({}, ts {})",
tableRef, timestamp)) {
return delegate().getRange(tableRef, rangeRequest, timestamp);
}
// No tracing, as we just return a lazy iterator and don't perform any calls to the backing KVS.
return delegate().getRange(tableRef, rangeRequest, timestamp);
}

@Override
public ClosableIterator<RowResult<Set<Long>>> getRangeOfTimestamps(TableReference tableRef,
RangeRequest rangeRequest,
long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getRangeOfTimestamps({}, ts {})",
tableRef, timestamp)) {
return delegate().getRangeOfTimestamps(tableRef, rangeRequest, timestamp);
}
// No tracing, as we just return a lazy iterator and don't perform any calls to the backing KVS.
return delegate().getRangeOfTimestamps(tableRef, rangeRequest, timestamp);
}

@Override
public ClosableIterator<List<CandidateCellForSweeping>> getCandidateCellsForSweeping(TableReference tableRef,
CandidateCellForSweepingRequest request) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getCandidateCellsForSweeping({}, ts {})",
tableRef, request.maxTimestampExclusive())) {
return delegate().getCandidateCellsForSweeping(tableRef, request);
}
// No tracing, as we just return a lazy iterator and don't perform any calls to the backing KVS.
return delegate().getCandidateCellsForSweeping(tableRef, request);
}

@Override
Expand All @@ -279,7 +278,7 @@ public Map<Cell, Value> getRows(TableReference tableRef,
long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getRows({}, {} rows, ts {})",
tableRef, Iterables.size(rows), timestamp)) {
LoggingArgs.safeTableOrPlaceholder(tableRef), Iterables.size(rows), timestamp)) {
return delegate().getRows(tableRef, rows, columnSelection, timestamp);
}
}
Expand All @@ -291,7 +290,7 @@ public Map<byte[], RowColumnRangeIterator> getRowsColumnRange(TableReference tab
long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getRowsColumnRange({}, {} rows, ts {})",
tableRef, Iterables.size(rows), timestamp)) {
LoggingArgs.safeTableOrPlaceholder(tableRef), Iterables.size(rows), timestamp)) {
return delegate().getRowsColumnRange(tableRef, rows, columnRangeSelection, timestamp);
}
}
Expand All @@ -302,11 +301,8 @@ public RowColumnRangeIterator getRowsColumnRange(TableReference tableRef,
ColumnRangeSelection columnRangeSelection,
int cellBatchHint,
long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("getRowsColumnRange({}, {} rows, {} hint, ts {})",
tableRef, Iterables.size(rows), cellBatchHint, timestamp)) {
return delegate().getRowsColumnRange(tableRef, rows, columnRangeSelection, cellBatchHint, timestamp);
}
// No tracing, as we just return a lazy iterator and don't perform any calls to the backing KVS.
return delegate().getRowsColumnRange(tableRef, rows, columnRangeSelection, cellBatchHint, timestamp);
}

@Override
Expand All @@ -323,7 +319,7 @@ public void multiPut(Map<TableReference, ? extends Map<Cell, byte[]>> valuesByTa
public void put(TableReference tableRef, Map<Cell, byte[]> values, long timestamp) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("put({}, {} values, ts {})",
tableRef, values.size(), timestamp)) {
LoggingArgs.safeTableOrPlaceholder(tableRef), values.size(), timestamp)) {
delegate().put(tableRef, values, timestamp);
}
}
Expand All @@ -332,15 +328,16 @@ public void put(TableReference tableRef, Map<Cell, byte[]> values, long timestam
public void putMetadataForTable(TableReference tableRef, byte[] metadata) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("putMetadataForTable({}, {} bytes)",
tableRef, (metadata == null) ? 0 : metadata.length)) {
LoggingArgs.safeTableOrPlaceholder(tableRef), (metadata == null) ? 0 : metadata.length)) {
delegate().putMetadataForTable(tableRef, metadata);
}
}

@Override
public void putMetadataForTables(Map<TableReference, byte[]> tableRefToMetadata) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("putMetadataForTables({})", tableRefToMetadata.keySet())) {
try (CloseableTrace trace = startLocalTrace("putMetadataForTables({})",
LoggingArgs.safeTablesOrPlaceholder(tableRefToMetadata.keySet()))) {
delegate().putMetadataForTables(tableRefToMetadata);
}
}
Expand All @@ -350,7 +347,7 @@ public void putUnlessExists(TableReference tableRef, Map<Cell, byte[]> values)
throws KeyAlreadyExistsException {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("putUnlessExists({}, {} values)",
tableRef, values.size())) {
LoggingArgs.safeTableOrPlaceholder(tableRef), values.size())) {
delegate().putUnlessExists(tableRef, values);
}
}
Expand All @@ -363,27 +360,28 @@ public boolean supportsCheckAndSet() {
@Override
public void putWithTimestamps(TableReference tableRef, Multimap<Cell, Value> values) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace(
"putWithTimestamps({}, {} values)", tableRef, values.size())) {
try (CloseableTrace trace = startLocalTrace("putWithTimestamps({}, {} values)",
LoggingArgs.safeTableOrPlaceholder(tableRef), values.size())) {
delegate().putWithTimestamps(tableRef, values);
}
}

@Override
public void truncateTable(TableReference tableRef) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("truncateTable({})", tableRef)) {
try (CloseableTrace trace = startLocalTrace("truncateTable({})",
LoggingArgs.safeTableOrPlaceholder(tableRef))) {
delegate().truncateTable(tableRef);
}
}

@Override
public void truncateTables(Set<TableReference> tableRefs) {
//noinspection unused - try-with-resources closes trace
try (CloseableTrace trace = startLocalTrace("truncateTables({})", tableRefs)) {
try (CloseableTrace trace = startLocalTrace("truncateTables({})",
LoggingArgs.safeTablesOrPlaceholder(tableRefs))) {
delegate().truncateTables(tableRefs);
}
}

}

21 changes: 21 additions & 0 deletions atlasdb-client/src/main/java/com/palantir/atlasdb/logging/LoggingArgs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@
* Always returns unsafe, until hydrated.
*/
public final class LoggingArgs {

@VisibleForTesting
static final TableReference PLACEHOLDER_TABLE_REFERENCE =
TableReference.createWithEmptyNamespace("{table}");

@Value.Immutable
public interface SafeAndUnsafeTableReferences {
SafeArg<List<TableReference>> safeTableRefs();
Expand Down Expand Up @@ -82,13 +87,29 @@ public static SafeAndUnsafeTableReferences tableRefs(Collection<TableReference>
.build();
}

public static Iterable<TableReference> safeTablesOrPlaceholder(Collection<TableReference> tables) {
//noinspection StaticPseudoFunctionalStyleMethod - Use lazy iterator.
return Iterables.transform(tables, LoggingArgs::safeTableOrPlaceholder);
}

/**
* Returns a safe or unsafe arg corresponding to the supplied table reference, with name "tableRef".
*/
public static Arg<String> tableRef(TableReference tableReference) {
return tableRef("tableRef", tableReference);
}

/**
* If table is safe, returns the table. If unsafe, returns a placeholder.
*/
public static TableReference safeTableOrPlaceholder(TableReference tableReference) {
if (logArbitrator.isTableReferenceSafe(tableReference)) {
return tableReference;
} else {
return PLACEHOLDER_TABLE_REFERENCE;
}
}

public static Arg<String> tableRef(String argName, TableReference tableReference) {
return getArg(argName, tableReference.toString(), logArbitrator.isTableReferenceSafe(tableReference));
}
Expand Down
Loading

0 comments on commit c1e21ee

Please sign in to comment.