Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.HashSet;
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -132,4 +133,75 @@ private boolean shouldLogAtDebug(AuditMessage auditMessage) {
return debugCmdSetRef.get()
.contains(auditMessage.getOp().toLowerCase(Locale.ROOT));
}

/**
* Utility class for building performance log strings.
*/
public static class PerformanceStringBuilder {
private final StringBuilder builder = new StringBuilder(128).append('{');
/**
* Appends metadata operation latency in milliseconds.
* @param nanos Latency in nanoseconds.
*/
public void appendMetaLatencyNanos(long nanos) {
append("metaLatencyMs", TimeUnit.NANOSECONDS.toMillis(nanos));
}

/**
* Appends whole operation latency in milliseconds.
* @param nanos Latency in nanoseconds.
*/
public void appendOpLatencyNanos(long nanos) {
append("opLatencyMs", TimeUnit.NANOSECONDS.toMillis(nanos));
}

/**
* Appends the size in bytes.
* @param bytes Size in bytes.
*/
public void appendSizeBytes(long bytes) {
append("sizeByte", bytes);
}

/**
* Appends the count.
* @param count The count value to be appended.
*/
public void appendCount(long count) {
append("count", count);
}

/**
* Appends a stream mode flag.
*/
public void appendStreamMode() {
append("streamMode", "true");
}

private void append(String name, long value) {
append(name, String.valueOf(value));
}

/**
* Appends a name-value pair to the log string.
* @param name Name of the metric.
* @param value Value of the metric.
*/
private void append(String name, String value) {
builder.append(name)
.append('=')
.append(value)
.append(", ");
}

public String build() {
final int length = builder.length();
if (length < 2) {
return "{}";
}
builder.setCharAt(length - 2, '}');
builder.setLength(length - 1);
return builder.toString();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,14 @@ public final class AuditMessage implements Message {
private final Throwable throwable;

private AuditMessage(String user, String ip, String op,
Map<String, String> params, String ret, Throwable throwable) {
Map<String, String> params, String ret, Throwable throwable,
String performance) {
this.user = user;
this.ip = ip;
this.op = op;
this.params = params;
this.ret = ret;
this.message = formMessage(user, ip, op, params, ret);
this.message = formMessage(user, ip, op, params, ret, performance);
this.throwable = throwable;
}

Expand Down Expand Up @@ -78,6 +79,7 @@ public static class Builder {
private String op;
private Map<String, String> params;
private String ret;
private String performance;

public Builder setUser(String usr) {
this.user = usr;
Expand Down Expand Up @@ -109,15 +111,23 @@ public Builder withException(Throwable ex) {
return this;
}

public Builder setPerformance(String perf) {
this.performance = perf;
return this;
}

public AuditMessage build() {
return new AuditMessage(user, ip, op, params, ret, throwable);
return new AuditMessage(user, ip, op, params, ret, throwable,
performance);
}
}

private String formMessage(String userStr, String ipStr, String opStr,
Map<String, String> paramsMap, String retStr) {
Map<String, String> paramsMap, String retStr,
String performanceMap) {
String perf = performanceMap != null && !performanceMap.isEmpty()
? " | perf=" + performanceMap : "";
return "user=" + userStr + " | ip=" + ipStr + " | " + "op=" + opStr
+ " " + paramsMap + " | " + "ret=" + retStr;

+ " " + paramsMap + " | ret=" + retStr + perf;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public enum S3GAction implements AuditAction {

//ObjectEndpoint
CREATE_MULTIPART_KEY,
CREATE_MULTIPART_KEY_BY_COPY,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call it COPY_MULTIPART_KEY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referencing the AWS API name

CREATE_MULTIPART_KEY: AWS API name UploadPart
CREATE_MULTIPART_KEY_BY_COPY: AWS API name UploadPartCopy

I think it's a better match, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Let's keep it then.

COPY_OBJECT,
CREATE_KEY,
LIST_PARTS,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
import java.util.List;
import java.util.Set;

import static org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
import static org.apache.hadoop.ozone.OzoneAcl.AclScope.ACCESS;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
import static org.apache.hadoop.ozone.s3.S3GatewayConfigKeys.OZONE_S3G_LIST_KEYS_SHALLOW_ENABLED;
Expand Down Expand Up @@ -113,6 +114,8 @@ public Response get(
@Context HttpHeaders hh) throws OS3Exception, IOException {
long startNanos = Time.monotonicNowNanos();
S3GAction s3GAction = S3GAction.GET_BUCKET;
PerformanceStringBuilder perf = new PerformanceStringBuilder();

Iterator<? extends OzoneKey> ozoneKeyIterator;
ContinueToken decodedToken =
ContinueToken.decodeFromString(continueToken);
Expand Down Expand Up @@ -264,12 +267,15 @@ public Response get(
response.setTruncated(false);
}

AUDIT.logReadSuccess(buildAuditMessageForSuccess(s3GAction,
getAuditParameters()));
int keyCount =
response.getCommonPrefixes().size() + response.getContents().size();
getMetrics().updateGetBucketSuccessStats(startNanos);
long opLatencyNs =
getMetrics().updateGetBucketSuccessStats(startNanos);
getMetrics().incListKeyCount(keyCount);
perf.appendCount(keyCount);
perf.appendOpLatencyNanos(opLatencyNs);
AUDIT.logReadSuccess(buildAuditMessageForSuccess(s3GAction,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AUDIT may not log reads because of performance, but the perf does always build(). Should the performance data be passed as a lamba instead, so that a new string is not actually built when read audi logs are not enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem that deserves attention
We currently generate AuditMessages via buildAuditMessageForSuccess, etc. In the AuditMessageForSuccess we perform the build of all relevant messages, regardless of whether or not the logs are logged.
This is probably not just a perf problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the problem also applies to the existing, "functional" part of the audit message.

Audit logging is enabled by default, and we need several changes to improve it, so I think we should do it separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reopened HDDS-2638 for the improvement of audit message processsing/allocations.

getAuditParameters(), perf.build()));
response.setKeyCount(keyCount);
return Response.ok(response).build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,14 @@ public AuditMessage buildAuditMessageForSuccess(AuditAction op,
return builder.build();
}

public AuditMessage buildAuditMessageForSuccess(AuditAction op,
Map<String, String> auditMap, String performance) {
AuditMessage.Builder builder = auditMessageBaseBuilder(op, auditMap)
.withResult(AuditEventStatus.SUCCESS);
builder.setPerformance(performance);
return builder.build();
}

@Override
public AuditMessage buildAuditMessageForFailure(AuditAction op,
Map<String, String> auditMap, Throwable throwable) {
Expand Down
Loading