Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add new IPC tags from IPC Metrics v2 spec #1155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
@@ -0,0 +1,44 @@
/**
* Copyright 2024 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.netflix.spectator.ipc;

import com.netflix.spectator.api.Tag;

public enum IpcAttemptReason implements Tag {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to overlap in large part with the existing ipc.attempt.


/**
* Represents the initial attempt for the request.
*/
initial,

/**
* Represents a retry attempt for the request.
*/
retry,

/**
* Represents a hedge attempt for the request.
*/
hedge;

@Override public String key() {
return IpcTagKey.attemptFinal.key();
}

@Override public String value() {
return name();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,17 @@ public final class IpcLogEntry {
private String protocol;

private IpcStatus status;
private String statusCode;
private String statusDetail;
private Throwable exception;

private IpcAttempt attempt;
private String attemptReason;
private IpcAttemptFinal attemptFinal;

private String vip;
private String endpoint;
private String method;

private String clientRegion;
private String clientZone;
Expand Down Expand Up @@ -217,6 +220,14 @@ public IpcLogEntry withStatus(IpcStatus status) {
return this;
}

/**
* Set the implementation specific status code for the request.
*/
public IpcLogEntry withStatusCode(String statusCode) {
this.statusCode = statusCode;
return this;
}

/**
* Set the detailed implementation specific status for the request. In most cases it
* is preferable to use {@link #withException(Throwable)} or {@link #withHttpStatus(int)}
Expand Down Expand Up @@ -282,6 +293,22 @@ public IpcLogEntry withAttempt(int attempt) {
return withAttempt(IpcAttempt.forAttemptNumber(attempt));
}

/**
* Set the reason for the attempt for the request.
* See {@link IpcAttemptReason} for possible values.
*/
public IpcLogEntry withAttemptReason(IpcAttemptReason attemptReason) {
return withAttemptReason(attemptReason.value());
}

/**
* Set the reason for the attempt for the request.
*/
public IpcLogEntry withAttemptReason(String attemptReason) {
this.attemptReason = attemptReason;
return this;
}

/**
* Set whether or not this is the final attempt for the request.
*/
Expand All @@ -307,6 +334,22 @@ public IpcLogEntry withEndpoint(String endpoint) {
return this;
}

/**
* Set the method used for this request.
* See {@link IpcMethod} for possible values.
*/
public IpcLogEntry withMethod(IpcMethod method) {
return withMethod(method.value());
}

/**
* Set the method used for this request.
*/
public IpcLogEntry withMethod(String method) {
this.method = method;
return this;
}

/**
* Set the client region for the request. In the case of the server side this will be
* automatically filled in if the {@link NetflixHeader#Zone} is specified on the client
Expand Down Expand Up @@ -899,6 +942,7 @@ public String toString() {
.addField("protocol", protocol)
.addField("uri", uri)
.addField("path", path)
.addField("method", method)
.addField("endpoint", endpoint)
.addField("vip", vip)
.addField("clientRegion", clientRegion)
Expand All @@ -916,9 +960,11 @@ public String toString() {
.addField("remoteAddress", remoteAddress)
.addField("remotePort", remotePort)
.addField("attempt", attempt)
.addField("attemptReason", attemptReason)
.addField("attemptFinal", attemptFinal)
.addField("result", result)
.addField("status", status)
.addField("statusCode", statusCode)
.addField("statusDetail", statusDetail)
.addField("exceptionClass", getExceptionClass())
.addField("exceptionMessage", getExceptionMessage())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/**
* Copyright 2024 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.netflix.spectator.ipc;

import com.netflix.spectator.api.Tag;

public enum IpcMethod implements Tag {

/**

Choose a reason for hiding this comment

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

Now that I look at this class, I'd like to collect feedback on if we should keep these common sets of values here or if we should treat each implementation as their own sets maintained by the implementation itself.

* Represents a unary gRPC method.
*/
unary,

/**
* Represents a client streaming gRPC method.
*/
client_streaming,

/**
* Represents a server streaming gRPC method.
*/
server_streaming,

/**
* Represents a bidirectional streaming gRPC method.
*/
bidi_streaming,

/**
* Represents an HTTP GET request.
*/
get,

/**
* Represents an HTTP POST request.
*/
post,

/**
* Represents an HTTP PUT request.
*/
put,

/**
* Represents an HTTP PATCH request.
*/
patch,

/**
* Represents an HTTP DELETE request.
*/
delete,

/**
* Represents an HTTP OPTIONS request.
*/
options,

/**
* Represents a GraphQL query.
*/
query,

/**
* Represents a GraphQL mutation.
*/
mutation,

/**
* Represents a GraphQL subscription.
*/
subscription;

@Override public String key() {
return IpcTagKey.method.key();
}

@Override public String value() {
return name();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ public enum IpcMetric {
IpcTagKey.status
),
EnumSet.of(
IpcTagKey.attemptReason,

Choose a reason for hiding this comment

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

Another idea here that we should get more feedback on is if we should :

  1. create a new set of IpcMetricV2 enums so we separate the existing tag sets from the new tag sets
  2. version this library and maintain the v1 separate from v2 (probably not enough here to warrant that)
  3. keep this as-is understanding that the optional tags from v1 and v2 are slightly different

IpcTagKey.endpoint,
IpcTagKey.method,
IpcTagKey.failureInjected,
IpcTagKey.httpMethod,
IpcTagKey.httpStatus,
Expand All @@ -58,6 +60,7 @@ public enum IpcMetric {
IpcTagKey.serverApp,
IpcTagKey.serverCluster,
IpcTagKey.serverAsg,
IpcTagKey.statusCode,
IpcTagKey.statusDetail,
IpcTagKey.vip
)
Expand All @@ -75,6 +78,7 @@ public enum IpcMetric {
),
EnumSet.of(
IpcTagKey.endpoint,
IpcTagKey.method,
IpcTagKey.clientApp,
IpcTagKey.clientCluster,
IpcTagKey.clientAsg,
Expand All @@ -83,6 +87,7 @@ public enum IpcMetric {
IpcTagKey.httpStatus,
IpcTagKey.id,
IpcTagKey.protocol,
IpcTagKey.statusCode,
IpcTagKey.statusDetail,
IpcTagKey.vip
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,31 @@ public enum IpcTagKey {
*/
result("ipc.result"),

/**
* Indicates where the result was ultimately sourced from such as cache, direct,
* proxy, fallback, etc.
*/
source("ipc.source"),

/**
* Dimension indicating a high level status for the request. These values are the same
* for all implementations to make it easier to query across services. See {@link IpcStatus}
* for permitted values.
*/
status("ipc.status"),

/**
* Dimension indicating the transport-specific code that aligns to the IPC status.
* The values for this are implementation specific. For example with HTTP,
* the status code value would be used here.
*/
statusCode("ipc.status.code"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't ipc.status.detail serve this purpose? I don't think we need yet another status dimension.


/**
* Optional dimension indicating a more detailed status. The values for this are
* implementation specific. For example with HTTP, the status code would be a likely
* value used here.
* implementation specific. For example, the {@link #status} may be
* {@code connection_error} and {@code statusDetail} would be {@code no_servers},
* {@code connect_timeout}, {@code ssl_handshake_failure}, etc.
*/
statusDetail("ipc.status.detail"),

Expand Down Expand Up @@ -72,6 +86,11 @@ public enum IpcTagKey {
*/
attempt("ipc.attempt"),

/**
* Indicates the reason for the attempt. See {@link IpcAttemptReason} for possible values.
*/
attemptReason("ipc.attempt.reason"),

/**
* Indicates if this is the final attempt for the request. For example, if the client
* is configured to allow 1 retry, then the second attempt would be final. Acceptable
Expand Down Expand Up @@ -103,6 +122,11 @@ public enum IpcTagKey {
*/
protocol("ipc.protocol"),

/**
* Method used to make the IPC request. See {@link IpcMethod} for possible values.
*/
method("ipc.method"),

/**
* Region where the client is located.
*
Expand Down
Loading
Loading