zipkin-instrumentation-express update typings #445
Conversation
Updating typings to expressMiddleware to include non-optional serviceName parameter
|
Thanks, I will check it by tomorrow!
tor. 12. sep. 2019, 23:50 skrev Jasmine Hegman <[email protected]>:
… Updating typings to expressMiddleware to include non-optional serviceName
parameter
------------------------------
You can view, comment on, or merge this pull request online at:
#445
Commit Summary
- zipkin-instrumentation-express update typings
File Changes
- *M* packages/zipkin-instrumentation-express/index.d.ts
<https://github.com/openzipkin/zipkin-js/pull/445/files#diff-0> (2)
Patch Links:
- https://github.com/openzipkin/zipkin-js/pull/445.patch
- https://github.com/openzipkin/zipkin-js/pull/445.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#445?email_source=notifications&email_token=AAXOYAXYOH6O3FRT47LJOILQJK2TPA5CNFSM4IWJ7542YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HLDJNFA>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXOYAXZAJMSK3D33NSUOVTQJK2TPANCNFSM4IWJ754Q>
.
|
|
Alternatively we should define it as optional, as it ultimately just passes through to https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin/src/instrumentation/httpServer.js#L31 and there it is optional. LMK and I can adjust this MR :) |
| @@ -13,7 +13,7 @@ import {Tracer} from 'zipkin'; | |||
| * go to the correct spans | |||
| */ | |||
| export declare function expressMiddleware( | |||
| options: {tracer: Tracer, port?: number} | |||
| options: {tracer: Tracer, serviceName: string, port?: number} | |||
There was a problem hiding this comment.
we intentionally don't document this as it is usually a mistake to have a difference between middleware service name and tracer. this causes graphs to mess up. You may notice that there are a few pull requests to change readmes to not use this. Also, I except once we figure out how deprecation works, we'd remove it eventually.
What impact is it causing to leave this out?
|
I am with @adriancole on this so I will close this PR for now and add the deprecation message in the options. In any case, thanks a lot @r4j4h for the willingness to contribute! |
|
Related to this: #457 |
Updating typings to expressMiddleware to include non-optional serviceName parameter