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

yarpc.Config: Option to disable observability middleware #1421

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

willhug
Copy link
Contributor

@willhug willhug commented Jan 29, 2018

Summary: In order to allow some users to customize how they attach
observability middleware (e.g. users that want to log stats with shard
key). This PR adds the ability to not add the built in observability
middleware on the dispatcher. By default it's currently always added.
This will allow users to input their own middleware for observability
explicitly. Since it would be a breaking change to remove it silently,
this needs to be an new option that explicitly disables the middleware..

@abhinav abhinav changed the title Add option to yarpc.Config to disable observability middleware. yarpc.Config: Option to disable observability middleware Jan 29, 2018
@codecov
Copy link

codecov bot commented Jan 29, 2018

Codecov Report

Merging #1421 into start-disableMetrics-part_1.0 will decrease coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@                         Coverage Diff                         @@
##           _start_-disableMetrics-part_1.0    #1421      +/-   ##
===================================================================
- Coverage                            90.69%   90.45%   -0.25%     
===================================================================
  Files                                  209      209              
  Lines                                10344    10249      -95     
===================================================================
- Hits                                  9382     9271     -111     
- Misses                                 638      645       +7     
- Partials                               324      333       +9
Impacted Files Coverage Δ
dispatcher.go 97.19% <100%> (+0.02%) ⬆️
encoding/thrift/inbound.go 64.61% <0%> (-28.83%) ⬇️
transport/http/config.go 86.66% <0%> (-7.46%) ⬇️
transport/grpc/outbound.go 75.15% <0%> (-1.28%) ⬇️
transport/tchannel/transport.go 81.3% <0%> (-0.84%) ⬇️
transport/tchannel/header.go 96.49% <0%> (-0.74%) ⬇️
transport/http/transport.go 96.96% <0%> (-0.51%) ⬇️
transport/tchannel/handler.go 75.21% <0%> (-0.42%) ⬇️
peer/peerlist/list.go 96.8% <0%> (-0.34%) ⬇️
transport/tchannel/options.go 96.66% <0%> (-0.31%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 254b756...a4bd8a6. Read the comment docs.


// DisableAutoObservabilityMiddleware is used to stop the dispatcher from
// automatically attaching observability middleware to all inbounds and
// outbounds. It is the assumption that if if this option is disabled the
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it is assumed

// outbounds. It is the assumption that if if this option is disabled the
// observability middleware is being inserted in the Inbound/Outbound
// Middleware.
DisableAutoObservabilityMiddleware bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop Auto? I think it's a bit verbose

Copy link
Contributor

Choose a reason for hiding this comment

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

Could get away with ExcludeObservabilityMiddleware

// outbounds. It is the assumption that if if this option is disabled the
// observability middleware is being inserted in the Inbound/Outbound
// Middleware.
DisableAutoObservabilityMiddleware bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Could get away with ExcludeObservabilityMiddleware

Copy link
Contributor

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

+1 to @kriskowal's suggestion

@willhug willhug changed the base branch from _start_-disableMetrics-part_1.0 to dev March 2, 2018 02:03
@willhug willhug merged commit ea24974 into dev Mar 2, 2018
@willhug willhug deleted the disableMetrics-part_1.0 branch March 2, 2018 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants