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

deprecate IpcTagKey entries that are not in spec #885

Merged
merged 1 commit into from May 21, 2021

Conversation

brharrington
Copy link
Contributor

Consistency should be a high priority for the common IPC
metrics and we should discourage additions that are not
part of the spec. As a first step, the existing entries
for the IpcTagKey enum that are not in the spec have been
deprecated.

Consistency should be a high priority for the common IPC
metrics and we should discourage additions that are not
part of the [spec]. As a first step, the existing entries
for the IpcTagKey enum that are not in the spec have been
deprecated.

[spec]: https://netflix.github.io/spectator/en/latest/ext/ipc/
@brharrington brharrington added this to the 0.131.0 milestone May 13, 2021
@jleibund
Copy link

One question from 886 about httpMethod and httpStatus:

  • Currently our gRPC doesn't report either of these
  • Although our gRPC is H2 method is only used, in practice in the swagger bridge and always POST
  • Also re gRPC the httpStatus isn't meaningful when compared with gRPC status codes
  • In similar fashion GraphQL doesn't leverage http status codes except for network errors - they are tunneled through 200 OK responses
  • It seems to me that http codes are valuable mostly for http-centric comms
  • If IPC metrics are truly common, what of UDP (DNS) etc or websockets

Just wondering, as part of this, can we document or annotate the httpStatus and httpMethod tags as applicable to http communication - some sort of caveat that they should be required when http or http-based but optional where less meaningful or when tagging a non-http (or even non-tcp) based IPC protocol?

@brharrington
Copy link
Contributor Author

Yeah, the http status/method stuff are optional and only intended for HTTP based requests. That should be clarified in the spec.

Copy link
Contributor

@kerumai kerumai left a comment

Choose a reason for hiding this comment

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

This seems reasonable. You may need to communicate this out to people/apps that currently publish the removed tags, before releasing this though.

@brharrington brharrington merged commit 38cbca9 into Netflix:master May 21, 2021
@brharrington brharrington deleted the ipc-nonspec branch May 21, 2021 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants