-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Bind this.handleApiError in PodDetail #84
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️ Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🎉
@@ -17,9 +17,9 @@ export default class Deployments extends React.Component { | |||
constructor(props) { | |||
super(props); | |||
this.api = ApiHelpers(this.props.pathPrefix); | |||
this.handleApiError = this.handleApiError.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice alphabetizing 🔤
The `metrics!` macro is currently local to the telemetry module. Furthermore, the `telemetry::metrics` module no longer has proxy-specific logic. This change moves the `telemetry::metrics` module into a new crate, `linkerd2_metrics`. This will enable unifying `telemetry::http` and `telemetry::transport` into `http` and `transport`, respectively.
Following linkerd#84, the `telemetry::transport` module can be moved into the `transport` module. This should allow us to simplify type signatures by combining redundant types. It's also hoped that we can reduce the API boilerplate around metrics so it's much easier to instrument and track new metrics in transport code.
In #65 I forgot to bind handleApiError in the PodDetail module, which resulted in this error if a fetch call failed:
This adds that binding.