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.

Sign up for GitHub

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

Jump to bottom

Add errors data to MethodMeta message #10

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add errors data to MethodMeta message #10

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b96486
Add errors field to MethodMeta that indicates client timeouts, server…
shreys7 Nov 2, 2023
e222150
Remove the connection reset error type and club it with client and se…
shreys7 Nov 3, 2023
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
479 changes: 321 additions & 158 deletions go/api_spec/method.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

28 changes: 28 additions & 0 deletions proto/api_spec/method.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,39 @@ message HTTPMethodMeta {
float processing_latency = 4;
}

message HTTPMethodError {
// Defines the possible error states we can reach while trying to generate a witness (request+response pair)
enum errorType {
// Represents the case where the agent timed out before it could create a complete witness.
AGENT_TIMEOUT = 0;

// Represents the case where the agent could not parse the packet as expected.
AGENT_PARSING_ERROR = 1;

// Represents the case where the client timed out waiting for a response from the server.
CLIENT_TIMEOUT = 2;

// Represents the case where the server timed out before it could send the response back.
SERVER_TIMEOUT = 3;

// Represents when one of the parties decides to abruptly terminate the connection.
// A reset is usually indicative of a problem, similar to a timeout, but the underlying causes can be different.
CONNECTION_RESET = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these three can be combined into two, or split into four, but they're a bit ambiguous right now.

We don't really know that the cause of a early close is, it might be a timeout or it might be a crash. So I think we could do CLIENT_CLOSED and SERVER_CLOSED and that covers all the cases, without the need for CONNECTION_RESET.

At the TCP protocol level we could distinguish between a normal FIN shutdown and an actual RST, but RST should be rare and I don't think it's necessarily worth calling them out separately. (RST tends to indicate that the TCP state machines are not in sync, for example if a machine rebooted and came back.) But the alternative is CLIENT_FIN, SERVER_FIN, CLIENT_RST, SERVER_RST to cover the full spectrum of what we could observe.


// Other unknown error observed while generating a witness.
OTHER = 5;
}

errorType type = 1;
string message = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to think a bit about whether we want to store this unstructured field.

Alternatives are:

  • Put it in the witness metadata (the JSON wrapper we use for upload) or client telemetry.
  • Use only structured data in witnesses.

The danger is that if we actually start relying on it then it becomes a fixed format we have to respect, and if we don't rely on it then what is the point of paying for it?

Do you have any thoughts here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what you mean when you say witness metadata. Do you mean somewhere in the witness with info struct?

type witnessWithInfo struct {
	// The name of the interface on which this witness was captured.
	netInterface string

	srcIP           net.IP // The HTTP client's IP address.
	srcPort         uint16 // The HTTP client's port number.
	dstIP           net.IP // The HTTP server's IP address.
	dstPort         uint16 // The HTTP server's port number.
	observationTime time.Time
	id              akid.WitnessID
	requestEnd      time.Time
	responseStart   time.Time

	witness *pb.Witness
}

I was also skeptical about adding this field. I kept it to pair it up with the OTHER errorType case where we don't know what exactly went wrong, but we do have an error message that can be used to display somewhere (to us or to the user). I am open for the option of omitting it completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's the structure I was thinking of. We do not have clear design guidelines about what is "info" and what belongs in the IR itself. I think I erred on my very first project by putting processing latency in the IR, when it would have been fine in witnessWithInfo instead.

I do think the error should be part of the witness, so that the witness clearly records its state, but I'm not 100% sure I'm not making the same error again. :)

We can leave the message field as-is for now, but leave a note suggesting additional fields if any structured data needs to be transmitted.

}

message MethodMeta {
oneof meta {
GRPCMethodMeta grpc = 1;
HTTPMethodMeta http = 2;
}
repeated HTTPMethodError errors = 3;
}

message Method {
Expand Down
121 changes: 112 additions & 9 deletions py/api_spec/method_pb2.py
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 7 additions & 7 deletions ts/api_spec/api_type_pb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@

var jspb = require('google-protobuf');
var goog = jspb;
var global = (function() {
if (this) { return this; }
if (typeof window !== 'undefined') { return window; }
if (typeof global !== 'undefined') { return global; }
if (typeof self !== 'undefined') { return self; }
return Function('return this')();
}.call(null));
var global =
(typeof globalThis !== 'undefined' && globalThis) ||
(typeof window !== 'undefined' && window) ||
(typeof global !== 'undefined' && global) ||
(typeof self !== 'undefined' && self) ||
(function () { return this; }).call(null) ||
Function('return this')();

goog.exportSymbol('proto.api_spec.ApiType', null, global);
/**
Expand Down
38 changes: 38 additions & 0 deletions ts/api_spec/method_pb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,39 @@ export namespace HTTPMethodMeta {
}
}

export class HTTPMethodError extends jspb.Message {
getType(): HTTPMethodError.errorType;
setType(value: HTTPMethodError.errorType): HTTPMethodError;
getMessage(): string;
setMessage(value: string): HTTPMethodError;

serializeBinary(): Uint8Array;
toObject(includeInstance?: boolean): HTTPMethodError.AsObject;
static toObject(includeInstance: boolean, msg: HTTPMethodError): HTTPMethodError.AsObject;
static extensions: {[key: number]: jspb.ExtensionFieldInfo<jspb.Message>};
static extensionsBinary: {[key: number]: jspb.ExtensionFieldBinaryInfo<jspb.Message>};
static serializeBinaryToWriter(message: HTTPMethodError, writer: jspb.BinaryWriter): void;
static deserializeBinary(bytes: Uint8Array): HTTPMethodError;
static deserializeBinaryFromReader(message: HTTPMethodError, reader: jspb.BinaryReader): HTTPMethodError;
}

export namespace HTTPMethodError {
export type AsObject = {
type: HTTPMethodError.errorType,
message: string,
}

export enum errorType {
AGENT_TIMEOUT = 0,
AGENT_PARSING_ERROR = 1,
CLIENT_TIMEOUT = 2,
SERVER_TIMEOUT = 3,
CONNECTION_RESET = 4,
OTHER = 5,
}

}

export class MethodMeta extends jspb.Message {

hasGrpc(): boolean;
Expand All @@ -998,6 +1031,10 @@ export class MethodMeta extends jspb.Message {
clearHttp(): void;
getHttp(): HTTPMethodMeta | undefined;
setHttp(value?: HTTPMethodMeta): MethodMeta;
clearErrorsList(): void;
getErrorsList(): Array<HTTPMethodError>;
setErrorsList(value: Array<HTTPMethodError>): MethodMeta;
addErrors(value?: HTTPMethodError, index?: number): HTTPMethodError;

getMetaCase(): MethodMeta.MetaCase;

Expand All @@ -1015,6 +1052,7 @@ export namespace MethodMeta {
export type AsObject = {
grpc?: GRPCMethodMeta.AsObject,
http?: HTTPMethodMeta.AsObject,
errorsList: Array<HTTPMethodError.AsObject>,
}

export enum MetaCase {
Expand Down
Loading