[WIP] Add new IPC tags from IPC Metrics v2 spec #1155
Conversation
| @@ -49,7 +49,9 @@ public enum IpcMetric { | |||
| IpcTagKey.status | |||
| ), | |||
| EnumSet.of( | |||
| IpcTagKey.attemptReason, | |||
There was a problem hiding this comment.
Another idea here that we should get more feedback on is if we should :
- create a new set of
IpcMetricV2enums so we separate the existing tag sets from the new tag sets - version this library and maintain the v1 separate from v2 (probably not enough here to warrant that)
- keep this as-is understanding that the optional tags from v1 and v2 are slightly different
|
|
||
| public enum IpcMethod implements Tag { | ||
|
|
||
| /** |
There was a problem hiding this comment.
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.
|
My thoughts on open questions:
I would say yeah. I also raised the question in the code comments if we should have a
Good question. We need more understanding from others on how these are used. I like having concrete values provided to limit inconsistency, but also, is the non-exhaustive list more confusing to knowing it can be any string?
This is tricky. If we don't end up versioning this library, then technically the aren't deprecated. I think if we go the route of |
Probably. I think we should try to provide as much clarity for use as possible.
I would argue that if it is worth standardizing, then we should strongly bias toward consistency and minimize any deviation. I'm not really sure I see the value of
To me the value of standardization is to be consistent and having different views of the IPC harms that goal. So if we are going to change them, then I would argue to try and get to a consistent place quickly and avoid long term issues with multiple versions. If that isn't possible, then it is probably better to not mess with the a new version of the existing IPC metrics and just do a minimal version of the api ipc metrics. |
| * The values for this are implementation specific. For example with HTTP, | ||
| * the status code value would be used here. | ||
| */ | ||
| statusCode("ipc.status.code"), |
There was a problem hiding this comment.
Why doesn't ipc.status.detail serve this purpose? I don't think we need yet another status dimension.
|
|
||
| import com.netflix.spectator.api.Tag; | ||
|
|
||
| public enum IpcAttemptReason implements Tag { |
There was a problem hiding this comment.
This seems to overlap in large part with the existing ipc.attempt.
Added new IPC tags as per the V2 spec:
Open questions for discussion:
ApiMetric(Similar to IpcMetric)IpcAttemptReasonandIpcMethodenums? The values present in the enum classes aren't supposed to be exhaustive hence I didn't put in a strict validation rule (As opposed to ipc.status which has a fixed set of allowed values)HttpMethodandHttpStatusfrom IpcTags